Skip to content

test(forge): solar test runner #2

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

Merged
merged 2 commits into from
May 8, 2025

Conversation

0xrusowsky
Copy link
Owner

@0xrusowsky 0xrusowsky commented May 7, 2025

this is what i'm trying to implement: foundry-rs#10405 (comment)

these are ok for now but I would like to import the test runner and logic from solar, which, like rustc, uses a directory walk and runs each file individually with custom annotations for config and expected errors; see https://docs.rs/ui_test/latest/ui_test/ and https://github.com/paradigmxyz/solar/blob/main/CONTRIBUTING.md#tests

I think we can just import and set up ui_test directly, and make it run forge lint on the file


  • added a new test util runner.rs, so that any crate can run "ui tests"
  • added a new flag with_json_emitter to SolidityLinter for testing purposes:
    impl Linter for SolidityLinter {
       type Language = SolcLanguage;
       type Lint = SolLint;
    
       fn lint(&self, input: &[PathBuf]) {
           let mut builder = Session::builder();
           let map = Arc::<SourceMap>::default();
    
           // Build session based on the linter config
           if self.with_json_emitter {
               let json_emitter = JsonEmitter::new(Box::new(std::io::stderr()), map.clone())
                   .rustc_like(true)
                   .ui_testing(false);
               let dcx = DiagCtxt::new(Box::new(json_emitter));
    
               builder = builder.dcx(dcx);
           } else {
               builder = builder.with_stderr_emitter();
           };
    
           // Create a single session for all files
           let mut sess = builder.source_map(map.clone()).build();
           sess.dcx = sess.dcx.set_flags(|flags| flags.track_diagnostics = false);
    
           // Process the files in parallel
           sess.enter_parallel(|| {
               input.into_par_iter().for_each(|file| {
                   self.process_file(&sess, file);
               });
           });
       }
    }
  • added the annotations to the solidity files to highlight the warnings/notes that the linter should emit

outputs

  • when i run the integration tests for the CLI cmds, i do see that the json emitter works properly:
{"$message_type":"diag","message":"","code":{"code":"divide-before-multiply","explanation":null},"level":"warning","spans":[{"file_name":"/private/var/folders/vq/wp74kqr523x_tp8sd4qgs35w0000gn/T/can_use_config-05tblQV/src/ContractWithLints.sol","byte_start":388,"byte_end":399,"line_start":16,"line_end":16,"column_start":9,"column_end":20,"is_primary":true,"text":[{"text":"        (1 / 2) * 3;","highlight_start":9,"highlight_end":20}],"label":null}],"children":[{"message":"multiplication should occur before division to avoid loss of precision","code":null,"level":"help","spans":[],"children":[],"rendered":null}],"rendered":"warning[divide-before-multiply]/n  [FILE]:16:9/n   |/n16 |         (1 / 2) * 3;/n   |         -----------/n   |/n   = help: multiplication should occur before division to avoid loss of precision/n/n"}
  • i can see that after running the ui tests setting args.bless = true, the .stderr files are generated (included in the PR) --> makes me think that the setup can't be too far from being correct.

however, when running cargo test -p forge --test ui none of the tests pass:

✗  cargo test -p forge --test ui                                                                                                                                                                                                                              [ 06:40:54 PM ]
   Compiling forge v1.1.0 (/Users/rusowsky/dev/ithaca/foundry/crates/forge)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 5.26s
     Running tests/ui/main.rs (target/debug/deps/ui-d11fb785c0016a76)

running 1 test
███████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████ 6/6test lint::forge_lint_ui_tests ... FAILED

failures:

---- lint::forge_lint_ui_tests stdout ----
Attempting to use forge executable at: /Users/rusowsky/dev/ithaca/foundry/target/debug/forge
Forge executable exists.
Forge --version stdout: forge Version: 1.1.0-dev
Commit SHA: 945ec589d0410617f564a5bfebc897610d3f1689
Build Timestamp: 2025-05-07T16:40:26.762823000Z (1746636026)
Build Profile: debug

Forge --version stderr:
Forge --version status: exit status: 0
ui_test root_dir: /Users/rusowsky/dev/ithaca/foundry/crates/lint/testdata
ui_test out_dir: /Users/rusowsky/dev/ithaca/foundry/crates/lint/target/ui
ui_test program path: /Users/rusowsky/dev/ithaca/foundry/target/debug/forge
ui_test program args: ["lint"]


FAILED TEST: /Users/rusowsky/dev/ithaca/foundry/crates/lint/testdata/Keccak256.sol
command: "/Users/rusowsky/dev/ithaca/foundry/target/debug/forge" "lint" "/Users/rusowsky/dev/ithaca/foundry/crates/lint/testdata/Keccak256.sol"

error: `asm-keccak256` not found in diagnostics on line 3
 --> /Users/rusowsky/dev/ithaca/foundry/crates/lint/testdata/Keccak256.sol:3:53
  |
3 |         keccak256(abi.encodePacked(a, b)); //~NOTE: asm-keccak256
  |                                                     ^^^^^^^^^^^^^ expected because of this pattern
  |

error: `asm-keccak256` not found in diagnostics on line 7
 --> /Users/rusowsky/dev/ithaca/foundry/crates/lint/testdata/Keccak256.sol:7:53
  |
7 |         keccak256(abi.encodePacked(a, b)); //~NOTE: asm-keccak256
  |                                                     ^^^^^^^^^^^^^ expected because of this pattern
  |

@0xrusowsky 0xrusowsky marked this pull request as ready for review May 8, 2025 08:07
@0xrusowsky 0xrusowsky merged commit e4b0a08 into feat/forge-lint May 8, 2025
19 of 21 checks passed
0xrusowsky added a commit that referenced this pull request May 23, 2025
* add lint cmd, variable lints

* wip

* wip

* wip

* wip

* wip

* wip

* add keccak256 opt test

* wip

* wip

* wip

* wip

* fix div before mul

* update lint args

* wip

* update declare lints macro

* update with_severity

* configure linter

* wip

* update hash value

* fix read in source

* rayon

* reorder lint declarations

* clippy

* add placeholder for additional lints

* more placeholders

* wip

* wip

* refactor into sol linter

* impl Linter for SolidityLinter

* fmt

* wip

* wip

* refactor lints into SolLint enum

* update lint trait

* wip

* wip

* wip

* wip

* wip

* update lint

* update forge lint to use ProjectLinter

* wip

* include/exclude files from linting

* linter output display note

* configure with severity and description

* fmt

* implementing display

* wip

* wip

* implement display for linter output, clippy fixes

* add note to update colors

* update linter output display

* remove todos, clean up comments

* clean up display

* update med finding color

* add optional help message

* display help message

* simplify lint args, make severity configurable

* updating lints, update tests

* add tests for info patterns, fix regex

* remove function mixed case

* doc comments

* clippy

* fmt

* reorganize, crate level docs

* fix info lints

* Use Solar daignostics instead of `LinterOutput` (foundry-rs#6)

* use solar diagnostics, remove unneeded types

* update diagnostic emission

* clippy

* set track daignostics to false

* display help message

* set level according to severity

* update descriptions to be more concise

* removed LinterError from lint trait

* early pass + tests

* fix: fmt + clippy

* fix: fmt + clippy

* fix: fmt + clippy

* fix: fmt + clippy

* fix: feedback

* fix: feedback

* fix: regex

Co-authored-by: zerosnacks <[email protected]>

* tests: cli integration

* fix: broken test

* fix: fmt

Co-authored-by: DaniPopes <[email protected]>

* style: naming

Co-authored-by: DaniPopes <[email protected]>

* style: fmt

Co-authored-by: DaniPopes <[email protected]>

* fix: use heck + individual lint macros + housekeeping

* fix: single session with parallel linting per file

* style: fix docs errors + typos

* docs: ref to deleted field

* fix: preprocessor regression + cargo.toml + default lint config tests

* test(forge): solar test runner (#2)

* style: clippy

* typo

Co-authored-by: DaniPopes <[email protected]>

* fix: housekeeping

* docs: linter docs for users + devs

* docs: style

* docs: style

* clone lint testdata with lf

* fix: out dir constructor

* update toml

* fix: merge conflicts

* fix: cargo.lock merge conflicts

* style: clippy

* style: whitespace

* fix: clippy

* Cargo.lock update to weekly task

* Preserve alloy patch placeholders

* Clippy

---------

Co-authored-by: 0xKitsune <[email protected]>
Co-authored-by: 0xKitsune <[email protected]>
Co-authored-by: zerosnacks <[email protected]>
Co-authored-by: DaniPopes <[email protected]>
Co-authored-by: grandizzy <[email protected]>
Co-authored-by: grandizzy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants