test: add JS fuzz/invariant integration tests

[agostbiro]

Add JS fuzz/invariant integration tests and fix the following issues:

• Make the failure persist dir optional for fuzz config as intended originally to be able to disable persisting results.
• Disable collecting gas report samples during fuzz/invariant testing for performance as we don't yet support gas reports.
• Remove unwrap on FuzzConfig::failure_persist_file. Instead reflect in the type system that it's not optional.
• Mitigate leaking the fuzz failure persist path by making sure that one particular path is only leaked once.

[changeset-bot]

⚠️ No Changeset found

Latest commit: ab94d0b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[Wodann]

One concern w.r.t. the leaking of static strings. Otherwise LGTM!

[Wodann]

For correctness, I believe we should still drop the leaked strings upon static deinitialization. That can be achieved by adding a local struct SomeName { inner: HashSet<&'static str> } which has a impl Drop. This destructor would be called when the static FAILURE_PATHS is dropped, guaranteeing that our application doesn't report any memory leaks in tooling.

[agostbiro]

Hey, I tried this out with LLVM leak sanitizer and it reports errors in the base branch, but not in this one, so I think it's ok as is.

Test command (tried on ARM linux in Docker): RUSTFLAGS="-Z sanitizer=leak" cargo +nightly test -p forge test_fuzz

[agostbiro]

(I couldn't get Miri working on the forge integration tests)

[Wodann]

Thanks for validating with leak sanitizer.

This is the reason why Leak Sanitizer would not be able to detect a "memory leak" of this kind. It depends on your definition of memory leak, though. Leak sanitizer doesn't consider it a leak because a static variable is still referring to the address of the originally allocated strings.

[fvictorio]

I pushed a commit removing chai-as-promised and checking if a directory exists using fs.existsSync.

The previous version wasn't bad or anything, but I've had so many bad experiences with chai-as-promised that I'd rather avoid it.

[fvictorio]

I would rename this to invariantConfig

[agostbiro]

Thanks, fixed in ab94d0b

[fvictorio]

I think there is a more robust way to check this. The TestResult objects have a kind.runs number for invariant tests which will be 1 when there is a cached seed, and it will almost surely be above 1 when there's not (and this can be done even more unlikely by changing the % 13 part of the solidity test to a bigger value).

The problem is that runTestsWithStats only returns the number of tests and failures. Maybe we can just add a testResults property to its returned value, that way there's no need to refactor all the existing usages of that helper.

This is just a suggestion btw, feel free to keep it as is if you think that this approach won't have frequent false positives or negatives.

[agostbiro]

Thanks, good idea, switched to checking runs. The assertion is that there are more than 1 runs on an uncached run and there is 1 run on a cached one. I didn't want to increase the 13 to a larger value to reduce the chance of the invariant testing not catching the bug.

ab94d0b