Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

meta(testing): migrate CLI / UI tests to snapbox #8389

Closed
DaniPopes opened this issue Jul 8, 2024 · 1 comment · Fixed by #8728
Closed

meta(testing): migrate CLI / UI tests to snapbox #8389

DaniPopes opened this issue Jul 8, 2024 · 1 comment · Fixed by #8728
Assignees
Labels
A-internals Area: internals first issue A good way to start contributing T-meta Type: meta
Milestone

Comments

@DaniPopes
Copy link
Member

DaniPopes commented Jul 8, 2024

Currently our CLI/UI tests are doing manual assertions with the output, manually parsing JSON and comparing against another manually parsed fixture file etc.

This is not ideal, and not very scalable as any change in the output will also require manually updating all the fixture files.

We should migrate to using the snapbox library which is also used by cargo in most of their tests.

Supersedes #7604

Unblocks #6569

The migration can be done gradually over multiple commits, however we also need an initial integration with the TestCommand to allow asserting with snapbox's str! and file! macros.

Completed in #8406

Migration guide

#8406 added helper assert() method to TestCommand, which executes configured command and returns snapbox's OutputAssert allowing to perform checks on output.

  • For tests which perform assertions on stdout, instead of asserting value of stdout_lossy/unchecked_stdout prefer using assert().stdout_eq(str![["<fixture>"]]) or assert_success().stdout_eq(str![["<fixture>"]]) if you need to assert successful status code.

    i.e the following test

    let output = cmd.args([...]).stdout_lossy();
    assert_eq!(stdout, "0x123");

    would become

    cmd.args([...]).assert_success().stdout_eq(str![["0x123"]]);
  • For tests using fixture files, prefer inlining those fixtures into str! macro invocations and removing the fixture files.

    i.e such assertion

    cmd.unchecked_output().stdout_matches_path(
        PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("path/to/fixture"),
    );

    would become

    cmd.args([...]).assert().stdout_eq(str![["<fixture file contents>"]])

    Large fixture files can be kept as is, prefer using file! macro for those.

  • Prefer replacing all irrelevant info in fixtures with snapbox filters (... as line wildcard, [..] as character-wildcard).
    i.e. if we're asserting on traces and output looks like this:

    Compiling 100 files with Solc 0.8.23
    Solc 0.8.23 finished in 14.35s
    Compiler run successful!
    
    Ran 1 test for test/Test.t.sol
    [PASS] test() (gas: 10000)
    Traces:
      [10000] TEST_CONTRACT::test()
        ├─ [0] VM::assertEq(1, 1) [staticcall]
        │  └─ ← [Return] 
        └─ ← [Stop] 
    

    the test would look similar to:

    cmd.arg("test").assert_success().stdout_eq(str![[r#"
    ...
    Traces:
      [10000] TEST_CONTRACT::test()
        ├─ [0] VM::assertEq(1, 1) [staticcall]
        │  └─ ← [Return] 
        └─ ← [Stop]
    "#]]);
  • Note: when writing tests it might be handy to set fixture to empty value (str![[""]]) and populate by running cargo test with SNAPSHOTS=overwrite to let snapbox populate fixtures automatically

@DaniPopes DaniPopes added the T-feature Type: feature label Jul 8, 2024
@mattsse mattsse added the first issue A good way to start contributing label Jul 9, 2024
@zerosnacks zerosnacks added the A-internals Area: internals label Jul 12, 2024
@0xjarix
Copy link

0xjarix commented Jul 24, 2024

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I am a security researcher so I have some experience with testing

How I plan on tackling this issue

I will follow your clear instructions and make the required changes to make the testing more scalable

@zerosnacks zerosnacks added this to the v1.0.0 milestone Jul 26, 2024
@zerosnacks zerosnacks self-assigned this Aug 8, 2024
@zerosnacks zerosnacks added T-meta Type: meta and removed T-feature Type: feature labels Aug 8, 2024
@zerosnacks zerosnacks changed the title Migrate CLI/UI tests to snapbox meta(testing): migrate CLI / UI tests to snapbox Aug 8, 2024
@zerosnacks zerosnacks added T-feature Type: feature and removed T-feature Type: feature labels Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-internals Area: internals first issue A good way to start contributing T-meta Type: meta
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants