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

fix(forge): Don't ignore config.toml when running invariant tests for coverage #6566

Merged
merged 3 commits into from
Dec 19, 2023

Conversation

0xmp
Copy link
Contributor

@0xmp 0xmp commented Dec 11, 2023

forge coverage ignores the config.toml file for invariant tests.

Invariant testing in coverage mode thus uses the default 256 runs which is small, making coverage for big invariant tests noisy/useless. This is not the case for regular (non-invariant) tests.

Motivation

This would bring us closer to having invariant tests as contributing fully to coverage (#4007).
Fixes #6565.

Solution

The coverage runner now takes the correct config given when running tests.

This is my first time writing Rust ever, so do feel free to ignore/rewrite completely.

@0xmp 0xmp changed the title fix(forge): Coverage for invariant tests no longer take default config fix(forge): Don't ignore config.toml when running invariant tests for coverage Dec 11, 2023
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! thanks for this!

So, I don't see an issue with the code itself—however, invariants right now don't contribute to coverage at all, so this would only really make coverage slower for users in the meanwhile :s. I don't think we wanna get this in before having a proper plan on how to revamp coverage and make invariant testing work with it.

@0xmp
Copy link
Contributor Author

0xmp commented Dec 13, 2023

invariants right now don't contribute to coverage at all

That's weird, I'm running an invariant test and getting some coverage info following this Twitter thread by using

forge coverage --report lcov --report-file test/myfile.lcov --match-contract myInvariantTest
genhtml ./test/myfile.lcov -o output

and then checking the output html.

The coverage does seem to increase on my test when I increase the number of runs. Maybe I'm getting "fooled by randomness" and it's just a question of seed or something?
Edit: I am using a Handler if that changes anything

@Evalir
Copy link
Member

Evalir commented Dec 14, 2023

Interesting thread—thanks for sharing. However we do not officially support coverage for invariant tests 😅 so while this might actually be working I'm really wary about this affecting CI to people. Wdyt @mds1 ?

@mds1
Copy link
Collaborator

mds1 commented Dec 14, 2023

Not sure if this is a common opinion, but:

  1. You should have good coverage even with 1 fuzz run and no invariant runs. This gives a stricter measure of your test coverage by ensuring you aren't relying on fuzzing/randomness.
  2. It is still useful to be able to see how much coverage many fuzz runs or many invariant runs provided, to gauge the effectiveness of your fuzz and invariant tests.

As a result, my preference is two coverage modes:

  1. One mode ignores config settings, sets fuzz runs to 1, ignores invariant tests.
  2. Second mode respects config settings,

I think people's expectation is that (2) is the default, and perhaps (1) becomes something like forge coverage --strict

I'm not sure where this PR fits into that potential design/UX

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's go with this for now—we'll respect the invariant settings, and we can add the --strict mode later.

@Evalir Evalir merged commit b9d9a5c into foundry-rs:master Dec 19, 2023
19 checks passed
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.

Forge coverage ignores config.toml when running invariant tests
3 participants