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.

Sign up for GitHub

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

Jump to bottom

testing: implement panicky testing.B struct #17

Closed
wants to merge 2 commits into from
Closed

testing: implement panicky testing.B struct #17

wants to merge 2 commits into from

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Oct 24, 2023

Fixes #16

Caveat: I haven't tested if it works, not sure what's the easiest way to do that (I suppose that there must exist easier ways than modding my whole oss-fuzz setup to use my fork).

I took nearly all public functions of testing.B, except func (b *B) RunParallel(body func(*PB)), which would pull in yet more structs.

I made the Run panic, otherwise don't see any need to make it panic. Someone might invoke ReportAllocs in init for all I know.

@AdamKorcz
Copy link
Owner

Thank you. I will have to test this before merging.

@holiman
Copy link
Contributor Author

holiman commented Oct 24, 2023 via email

@AdamKorcz
Copy link
Owner

It might be a problem to add TB because of this: https://github.com/golang/go/blob/983d90e11dd0e12a484def454530fe9f5de031fc/src/testing/testing.go#L900

@holiman
Copy link
Contributor Author

holiman commented Oct 24, 2023

It might be a problem to add TB ...

I don't think so. I mean, we're not actually implementing that interface, we just want to tick as many boxes in the testing package as possible, so that when we rewrite testing into the "github.com/AdamKorcz/go-118-fuzz-build/testing", we get as few compiler errors as possible.

Example: https://github.com/ethereum/go-ethereum/blob/a8617c6d4dbe0df8c67e1f5c2ecd76c3200dc18d/p2p/discover/v5wire/encoding_test.go#L524C2-L524C2

The TB is just used so that the method can be invoked both by regular tests and benchmarks, and if we're trying to make so we don't choke on B, then we should make it not choke on TB either.

@AdamKorcz
Copy link
Owner

Sg. Let me know when it is ready to be tested (and whether you have also tested it)

@holiman
Copy link
Contributor Author

holiman commented Oct 25, 2023

Yep, this is ready to be tested (but I have not yet done so)

@holiman
Copy link
Contributor Author

holiman commented Oct 25, 2023

I don't really see how this PR can break anything, but if it gets merged, I can test it out pretty quickly.

Alternatively, and probably better: if you put it in a branch, my oss-script can use it via go get xx@branch. It's just a huge hassle to install branch on a forked repo, because then I can't use go get, but need to install via source, and add remotes etc.

@AdamKorcz
Copy link
Owner

I will test this PR this week.

@holiman
Copy link
Contributor Author

holiman commented Nov 6, 2023

So, this B implementation is missing N, which needs to be added to the struct.

For my own purposes, I ended up implementing an alternative: https://github.com/holiman/gofuzz-shim.

It is corpus-incompatible with go-118-fuzz-build, since I realized that I wanted to totally change how the input from libfuzzer is used. That part is implemented here: https://github.com/holiman/gofuzz-shim/blob/main/input/reader.go#L102

Also, I did away with a lot of the code in the shimmer, making it so that the caller simply specifies what files to instrument.

This is the B I wound up using: https://github.com/holiman/gofuzz-shim/blob/main/testing/b.go . It uses a common which is shared with T -- much like the actual testing.T and testing.B under the hood. Feel free to use it if you want.

howardjohn added a commit to howardjohn/istio that referenced this pull request Jan 22, 2024
@howardjohn
fuzz: fix build
205b06f 
A bug in the infra breaks if a fuzzer and benchmark is in one folder

See AdamKorcz/go-118-fuzz-build#17

Split it out
@howardjohn howardjohn mentioned this pull request Jan 22, 2024
Merged
istio-testing pushed a commit to istio/istio that referenced this pull request Jan 29, 2024
@howardjohn
fuzz: fix build (#48909)
c72446e 
* fuzz: fix build

A bug in the infra breaks if a fuzzer and benchmark is in one folder

See AdamKorcz/go-118-fuzz-build#17

Split it out

* oops
liwenhao0810 pushed a commit to liwenhao0810/istio that referenced this pull request Feb 1, 2024
@howardjohn @liwenhao0810
fuzz: fix build (istio#48909)
c58c67f 
* fuzz: fix build

A bug in the infra breaks if a fuzzer and benchmark is in one folder

See AdamKorcz/go-118-fuzz-build#17

Split it out

* oops
thedebugger pushed a commit to thedebugger/istio that referenced this pull request Mar 5, 2024
@howardjohn @thedebugger
fuzz: fix build (istio#48909)
abf9892 
* fuzz: fix build

A bug in the infra breaks if a fuzzer and benchmark is in one folder

See AdamKorcz/go-118-fuzz-build#17

Split it out

* oops
@holiman holiman closed this Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing testing.B
2 participants
@holiman @AdamKorcz