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

cargo bolero list should not need to run the whole test suite #196

Open
Ekleog-NEAR opened this issue Nov 9, 2023 · 6 comments
Open

cargo bolero list should not need to run the whole test suite #196

Ekleog-NEAR opened this issue Nov 9, 2023 · 6 comments

Comments

@Ekleog-NEAR
Copy link
Contributor

I remember discussing this at some point, but somehow cannot find it in github any longer, so I’m opening this issue to track it :)

cargo bolero list currently needs to run the whole test suite to find the fuzzers.

This is suboptimal. A possible solution would be:

  • Replace bolero::check!() with a #[bolero::test] annotation
  • This annotation, in addition to adding the test as a cargo test, also adds a magic value to a specific segment of the ELF binary
  • cargo bolero list builds the binaries just like for fuzzing (this’d also remove one more compilation step, as list then test currently needs two compilations), and then reads these segments to list the fuzzers

cargo bolero list will probably need to depend on goblin or similar, in order to implement this.

@nagisa
Copy link

nagisa commented Nov 10, 2023

cargo test allows for only listing tests without running them (with a nice JSON output), so heuristically a large majority of the tests/fuzzers not applicable could be filtered out by just appending some well-known string to the test names in the bolero::check! macro. From there on the remaining tests could still be run to filter them out fully if desired.

@Ekleog-NEAR
Copy link
Contributor Author

@camshaft What do you think of this solution? It sounds reasonable to me, the only drawback being that the test name is no longer as free as a unit test, but must end with eg. _fuzzer.

If going this way, I’d argue that bolero::check! should error out if the test name doesn’t already end with _fuzzer, so that the test name still be the same in the test binary as in the source code.

@Ekleog-NEAR
Copy link
Contributor Author

Ekleog-NEAR commented Nov 10, 2023

Actually it wouldn’t solve the issue, I just tried making a quick prototype, and integration tests are run regardless of what filter is passed to cargo test, and are not reported by cargo test -- --list. So it sounds like the only good solution would be to actually implement the dump-to-section-and-parse-for-list solution :/

@nagisa
Copy link

nagisa commented Nov 10, 2023

integration tests are run regardless of what filter is passed to cargo test, and are not reported by cargo test -- --list.

Ah, right, this is something that will happen with custom harnesses. I wonder what nextest does here, since it seems to be able to list tests without running them despite the harness = false.

@nagisa
Copy link

nagisa commented Nov 10, 2023

Ah, https://nexte.st/book/custom-test-harnesses.html is the documentation on the interface here. Would it be too terrible to require a similar interface for bolero?

Ekleog-NEAR added a commit to Ekleog-NEAR/nearcore that referenced this issue Nov 14, 2023
See discussion at [1]. Here, the solution is what would be a bad
solution for bolero upstream (because it no longer works with
un-harnessed fuzzers), but is fine enough for our use case.

[1] camshaft/bolero#196
github-merge-queue bot pushed a commit to near/nearcore that referenced this issue Nov 15, 2023
See discussion at [1]. Here, the solution is what would be a bad
solution for bolero upstream (because it no longer works with
un-harnessed fuzzers), but is fine enough for our use case.

[1] camshaft/bolero#196
@camshaft
Copy link
Owner

I still think the most reliable method would be putting the harnesses in a custom linker section. Unfortunately, the check! macro doesn't actually know what the caller function name is at compile-time (only module_path!() is available), which means we'd probably need to change the harness interface to something like #39.

I'd prefer not to enforce naming conversations, TBH. That being said, one thing we can do in the short term is allow the caller to pass a filter to the list command, similar to what you've done in Ekleog-NEAR@8f4e49d, defaulting to no filter. This would allow folks that do have a naming convention to just target bolero harnesses.

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

No branches or pull requests

3 participants