-
Notifications
You must be signed in to change notification settings - Fork 16
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
test: add JS fuzz/invariant integration tests #658
Merged
agostbiro
merged 6 commits into
feat/solidity-tests
from
test/fuzz-js-integration-tests
Sep 13, 2024
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
e1c91f5
test: add isolate mode JS integration test
agostbiro bdfc572
Simplify isolate mode test and add inverse test
agostbiro 8a9bd9a
test: add JS fuzz/invariant integration tests
agostbiro db7cb92
Fix linter
agostbiro 7fe8315
Remove chai-as-promised
fvictorio ab94d0b
Address review feedback
agostbiro File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 aimpl Drop
. This destructor would be called when thestatic FAILURE_PATHS
is dropped, guaranteeing that our application doesn't report any memory leaks in tooling.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I couldn't get Miri working on the
forge
integration tests)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.