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

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

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

ahpaleus
Copy link
Contributor

@ahpaleus ahpaleus commented Jan 20, 2023

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

Xenomega and others added 16 commits December 11, 2022 02:51
# Conflicts:
#	chain/test_chain.go
#	chain/test_chain_test.go
#	fuzzing/fuzzer.go
… tx gas limit

* Change contract deployments to not need a deployment order if only one compiled contract is found
…orpus entries can be deep cloned

* Added tx-level corpus mutations powered by the ValueGenerator
… to CallMessageDataAbiValues.Resolve(), renamed a variable in abi_values.go
@ahpaleus ahpaleus requested a review from Xenomega January 20, 2023 13:28
…-assert

# Conflicts:
#	fuzzing/calls/call_sequence.go
#	fuzzing/fuzzer.go
#	fuzzing/fuzzer_worker_sequence_generator.go
#	fuzzing/test_case_assertion.go
#	fuzzing/valuegeneration/abi_values.go
#	fuzzing/valuegeneration/abi_values_test.go
@Xenomega Xenomega changed the base branch from dev/call-sequence-mutations to master January 24, 2023 21:53
@Xenomega
Copy link
Member

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 :)

@ahpaleus
Copy link
Contributor Author

ahpaleus commented Jan 26, 2023

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
}

@Xenomega
Copy link
Member

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(); }.

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

Successfully merging this pull request may close these issues.

require() instead of assert() in specific test cases
2 participants