native go fuzzing: ignore non-fuzz tests

[pjbgf]

Co-located tests that depend on global vars break the process of
building Fuzz tests.

A common scenario is to have a TestMain func specified on a separate
file, which defines and loads global vars to be shared across different.
Those test funcs are completely irrelevant to the Fuzz tests, as the
latter tend to be self-sufficient.

gofmt is then used to clean-up any unused import, so that Go compiler
is happy.

[AdamKorcz]

@pjbgf Thank you for the PR.

Nice trick with gofmt to clean up unused imports.

Regarding the sed command: Could you share an examples of a "before" code example and a desired/expected "after" code example?

Do you have a link to an example of a co-located test that breaks the OSS-Fuzz build?

[pjbgf]

@AdamKorcz sure, I linked the PR with the example, it has the file helmrelease_controller_test.go which contains both normal tests and fuzz tests.

One of the normal test depends on the k8sClient variable that is defined in a completely different file, therefore breaking the fuzz build.

The sed command clears out all the code within the non-fuzz funcs, resulting in:

func TestHelmReleaseReconciler_composeValues(t *testing.T) {
}

func TestValuesReferenceValidation(t *testing.T) {
}

Removing any dependency that is not related to the Fuzz tests.

[pjbgf]

Rebased with latest from master.

[AdamKorcz]

@AdamKorcz sure, I linked the PR with the example, it has the file helmrelease_controller_test.go which contains both normal tests and fuzz tests.

One of the normal test depends on the k8sClient variable that is defined in a completely different file, therefore breaking the fuzz build.

The sed command clears out all the code within the non-fuzz funcs, resulting in:

func TestHelmReleaseReconciler_composeValues(t *testing.T) {
}

func TestValuesReferenceValidation(t *testing.T) {
}

Removing any dependency that is not related to the Fuzz tests.

Thank you for the elaboration. I do see the problem here.

To me, there are side effects to having this sed command in the build script. If this gets merged, the build script will empty the body of all functions that has testing.T as its last parameter. However, some fuzz tests might want to use such functions. For example, this PR would break a fuzzer like this:

var testDB dbStruct
func setupDB(name string, t *testing.T) {
    testDB = NewDB(name)
}

func Fuzzer(f *testing.F) {
    f.Fuzz(func(t *testing.T, data []byte) {
        // create new db
        setupDB("newDB", t)
        // remaining fuzzing logic below
        ........
    })
}

In general, there are reasons that projects might want to have functions with the testing.T parameter in the same file as fuzzers besides those being separate unit tests. I think it will break things for other projects to force this behaviour on users in the build script. I could see this being useful if:

1. The build script checks if the functions with the testing.T parameter are used by the fuzzer (including transitively). or:
2. Users can enable this with a build tag or command-line parameter.

Option 1 is way too complex IMO, and I think it would be much simpler to let users to it themselves in the build script when they actually need it.

@pjbgf Let me know what you think.

[pjbgf]

@AdamKorcz thank you for your time reviewing this.

I made some changes to align with the option 2 you mentioned above. I added a boolean parameter to compile_native_go_fuzzer which defaults to false.

The sed command will also ensure the funcs its cleans up start with func Test and end with testing.T) {, to decrease the odds of it impacting helper funcs - like the example you shared.