medusa icon indicating copy to clipboard operation
medusa copied to clipboard

Use require package instead of assert while performing fs operations in unit tests

Open ahpaleus opened this issue 2 years ago • 3 comments

Instead of using the assert package, use the require package to stop testing if a test fails while performing filesystem operations (the assert package will only log the failure and continue execution).

Closes #86

I created a semgrep rule for this if we want to incorporate this tool into our CI.

rules:
  - id: require-instead-of-assert
    patterns:
      - focus-metavariable: $E
      - pattern-either:
          - pattern:
              $X, $E = $FSPACKAGE.$FUNC(...)
          - pattern:
              $E = $FSPACKAGE.$FUNC(...)
      - pattern-either:
        - pattern-inside: |
            ...
            
            assert.$Y(..., $X)
        - pattern-inside: |
            ...
            
            assert.$Y(..., $E)
      - metavariable-pattern:
          metavariable: $FSPACKAGE
          patterns:
            - pattern-either:
                - pattern: os
                - pattern: utils # medusa specific
                - pattern: ioutil
                - pattern: filepath
    message: Instead of using the `assert` package on the $E error variable, use the `require` package to stop testing
      if a test fails while performing filesystem operations.
    languages: [go]
    severity: ERROR

ahpaleus avatar Jan 20 '23 13:01 ahpaleus

I updated the branch here to target master.

Upon testing, we can't use require statements everywhere (in subtests, as it will FailNow() and then subsequent tests will fail for unrelated reasons: can't clean up the test's TempDir). We can probably use it in a few more places than this, but before merging this, I want to know why in the following code:

				// Start the fuzzer
				err := f.fuzzer.Start()
				assert.NoError(t, err)

				// Check for any failed tests and verify coverage was captured
				assertFailedTestsExpected(f, true)
				assertCorpusCallSequencesCollected(f, true)

The first assert.NoError() statement will fail an assertion, but the output of the failed assertion is not actually output if subsequent assertions (the two calls below) fail. That makes it a bit hard to capture the actual error on a failure.

I'll circle back to solving that, and then we can merge it. If you see an obvious solution (that doesn't require an if err != nil { continueProcessing(); }), then feel free to tackle it :)

Xenomega avatar Jan 25 '23 15:01 Xenomega

The first assert.NoError() statement will fail an assertion, but the output of the failed assertion is not actually output if subsequent assertions (the two calls below) fail. That makes it a bit hard to capture the actual error on a failure.

I'll circle back to solving that, and then we can merge it. If you see an obvious solution (that doesn't require an if err != nil { continueProcessing(); }), then feel free to tackle it :)

When the f.fuzzer.Events.FuzzerStopping event occurs, we can avoid printing output from the assert.NoError(t, err) and print only the two last ones.

Rough, but let me know what you think. fuzzing/fuzzer_test.go

func Test3(t *testing.T) {
	runFuzzerTest(t, &fuzzerSolcFileTest{
		filePath: "testdata/contracts/deployments/inner_inner_deployment.sol",
		configUpdates: func(config *config.ProjectConfig) {
			config.Fuzzing.DeploymentOrder = []string{"InnerDeploymentFactory"}
			config.Fuzzing.Workers = 50
			config.Fuzzing.TestLimit = 10000
		},
		method: func(f *fuzzerTestContext) {
			// Start the fuzzer
			err := f.fuzzer.Start()
			err = fmt.Errorf("not very important bug happened")

			expect := expectEventEmittedReturn(f, &f.fuzzer.Events.FuzzerStopping)
			if !expect {
				// Fuzzer did not stop properly, something bug happened
				assert.NoError(t, err)
			}

			// Check for any failed tests and verify coverage was captured
			if expect {
				// Fuzzer stopped properly, and only these assertions matter
				assertFailedTestsExpected(f, true)
				assertCorpusCallSequencesCollected(f, true)
			}

		},
	})
}

func expectEventEmittedReturn[T any](f *fuzzerTestContext, eventEmitter *events.EventEmitter[T]) bool {
	eventType := eventEmitter.EventType().String()

	eventEmitter.Subscribe(func(event T) error {
		f.eventCounter[eventType] += 1
		return nil
	})

	if f.eventCounter[eventType] == 0 {
		return false
	}

	return true
}

ahpaleus avatar Jan 26 '23 16:01 ahpaleus

I think this is a weird model for unit testing, to have to switch on each failure and not execute further code, so I'd prefer to find another solution here if we can. It was kind of the scenario I was trying to avoid with my comment regarding if err != nil { continueProcessing(); }.

Xenomega avatar Jan 26 '23 18:01 Xenomega