Skip to content

add sometimes assertions#803

Open
samalws-tob wants to merge 8 commits into
masterfrom
feat/sometimes-tests
Open

add sometimes assertions#803
samalws-tob wants to merge 8 commits into
masterfrom
feat/sometimes-tests

Conversation

@samalws-tob

@samalws-tob samalws-tob commented Feb 20, 2026

Copy link
Copy Markdown
Contributor

VIBE CODED PROTOTYPE. Still need to check whether this is correct, but it at least looks plausible
Also not completely sure that this interface is the best way to go, rather than adding assert statements

resolves #450


Adds support for "sometimes assertions" - a new test type that validates code paths are reached with sufficient frequency during fuzzing campaigns. This addresses issue #450 by providing "fuzz canaries" that alert when test harnesses become ineffective or regress.

Sometimes assertions are methods with a configurable prefix (default: "sometimes_") that take no arguments and should succeed (not revert) at least a minimum percentage of the time. If the success rate falls below the configured threshold after sufficient executions, the test fails.

Key features:

  • Configurable success rate threshold (default: 5%)
  • Minimum execution count before evaluation (default: 100)
  • Thread-safe statistics tracking across parallel workers
  • End-of-campaign evaluation to ensure statistical significance
  • Per-method execution and success counts in test output

Implementation:

  • New SometimesTestCaseProvider following the existing provider pattern
  • Configuration in FuzzingConfig.Testing.SometimesTesting
  • Statistics tracked per test: executionCount, successCount
  • Tests executed after each call in a sequence (no shrinking)
  • Success determined by: successRate >= minSuccessRate

Example usage:

function doSomething(uint256 x) public {
    if (x > 100) { counter++; }
}

// Passes if counter increments at least 5% of the time
function sometimes_counterIncremented() public view {
    require(counter > 0);
}

Includes comprehensive test coverage validating success rate calculations, minimum execution enforcement, and multi-contract scenarios.

samalws-tob and others added 2 commits March 10, 2026 11:42
Adds support for "sometimes assertions" - a new test type that validates
code paths are reached with sufficient frequency during fuzzing campaigns.
This addresses issue #450 by providing "fuzz canaries" that alert when
test harnesses become ineffective or regress.

Sometimes assertions are methods with a configurable prefix (default:
"sometimes_") that take no arguments and should succeed (not revert) at
least a minimum percentage of the time. If the success rate falls below
the configured threshold after sufficient executions, the test fails.

Key features:
- Configurable success rate threshold (default: 5%)
- Minimum execution count before evaluation (default: 100)
- Thread-safe statistics tracking across parallel workers
- End-of-campaign evaluation to ensure statistical significance
- Per-method execution and success counts in test output

Implementation:
- New SometimesTestCaseProvider following the existing provider pattern
- Configuration in FuzzingConfig.Testing.SometimesTesting
- Statistics tracked per test: executionCount, successCount
- Tests executed after each call in a sequence (no shrinking)
- Success determined by: successRate >= minSuccessRate

Example usage:
```solidity
function doSomething(uint256 x) public {
    if (x > 100) { counter++; }
}

// Passes if counter increments at least 5% of the time
function sometimes_counterIncremented() public view {
    require(counter > 0);
}
```

Includes comprehensive test coverage validating success rate calculations,
minimum execution enforcement, and multi-contract scenarios.

Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Replace the per-worker contract tracking maps and event handlers
(onWorkerCreated, onWorkerDeployedContractAdded, onWorkerDeployedContractDeleted)
with direct iteration over worker.DeployedContracts() in callSequencePostCallTest.
Move the shared testCasesLock onto each SometimesTestCase so workers testing
different methods don't contend with each other.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>
@samalws-tob
samalws-tob force-pushed the feat/sometimes-tests branch from 6fb144a to 38edabe Compare March 10, 2026 16:42
@samalws-tob

Copy link
Copy Markdown
Contributor Author

yeah this should be good

@samalws-tob
samalws-tob marked this pull request as ready for review March 10, 2026 16:53
@samalws-tob samalws-tob changed the title prototype: add sometimes assertions add sometimes assertions Mar 10, 2026
anishnaik and others added 2 commits March 12, 2026 14:55
P1 — Insufficient executions silently marked as PASSED:
  - Log a warning via GlobalLogger when a sometimes test does not reach
    minExecutionCount and is marked as passed by default. Users now see
    which tests were not fully evaluated.

P2 — Race condition on SometimesTestCase field reads:
  - Add lock acquisition to Status() and LogMessage() so concurrent
    workers cannot produce torn reads of executionCount/successCount.
  - Rename SuccessRate() to unexported successRate() (caller-holds-lock
    contract) and update all call sites.

P2 — StopOnFailedTest limitation undocumented:
  - Add doc comment on SometimesTestingConfig noting that sometimes tests
    are evaluated post-campaign, so StopOnFailedTest has no effect.

P2 — StopOnNoTests error message omits "sometimes":
  - Update both error strings in fuzzer.go to include "sometimes".

P3 — Test improvements:
  - TestSometimesMultipleContracts: assert zero failures, add explicit
    StopOnFailedTest = false for consistency with TestSometimesMode.
  - TestSometimesMode: use GreaterOrEqual to match the >= semantics in
    onFuzzerStopping.
  - Fix staticcheck QF1004: use strings.ReplaceAll in ID().

Dismissed findings:
  - P2 #4 (self-destructed contracts inflate success rate): real concern
    but low-probability edge case; fixing requires adopting per-worker
    contract lifecycle tracking which is a larger refactor best done in a
    follow-up.
  - P2 #5 (default config enables sometimes testing): consistent with
    property/optimization defaults; flagging for release notes instead.
  - P3 #8 (prefix overlap uses exact match): pre-existing limitation
    across all providers, not introduced by this PR.
  - P3 #9 (IsSometimesTest allows return values): by design — return
    values are harmless and not read. Documenting would add noise.
  - P3 #11 (failedSequences metric not incremented): inherent to
    post-campaign evaluation; no worker exists to attribute the metric to.
  - P4 findings: informational, no changes needed.

Verification: build, lint (zero new issues), go vet, and both
TestSometimesMode and TestSometimesMultipleContracts pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@anishnaik

Copy link
Copy Markdown
Collaborator

Review Summary

Findings

P1 — Blocks Merge

# Severity Finding Resolution
1 P1 Insufficient executions silently marked as PASSED — when minExecutionCount isn't reached, onFuzzerStopping marks the test as PASSED with no warning. A test with 0% success rate passes if the campaign is too short. Fixed: log a warning via GlobalLogger when a sometimes test is marked as passed due to insufficient executions

P2 — Important

# Severity Finding Resolution
2 P2 Race condition on SometimesTestCase field reads — Status(), SuccessRate(), LogMessage() read status/executionCount/successCount without the lock while workers concurrently update them Fixed: added lock acquisition to Status() and LogMessage(); renamed SuccessRate() to unexported successRate() with caller-holds-lock contract
3 P2 StopOnFailedTest ineffective for sometimes tests — failures only detected post-campaign, so StopOnFailedTest never triggers early exit Fixed: added doc comment on SometimesTestingConfig documenting this limitation
4 P2 Self-destructed contracts could inflate success rate — calling a test method on a self-destructed contract returns success at EVM level Dismissed: low-probability edge case; fixing requires per-worker contract lifecycle tracking (larger refactor for follow-up)
5 P2 Default config enables sometimes testing — could silently reclassify existing sometimes_-prefixed methods from assertion to sometimes tests on upgrade Dismissed: consistent with property/optimization defaults; should be noted in release notes
6 P2 StopOnNoTests error message omits "sometimes" Fixed: updated both error strings in fuzzer.go
7 P2 TestSometimesMultipleContracts doesn't assert zero failures Fixed: added failedTests assertion and explicit StopOnFailedTest = false

P3 — Nice to Have

# Severity Finding Resolution
8 P3 No per-worker state / hot-path allocation via DeployedContracts() on every call Dismissed: performance concern for follow-up; current approach is functionally correct
9 P3 Prefix overlap validation uses exact match, not substring containment Dismissed: pre-existing limitation across all providers
10 P3 IsSometimesTest allows methods with return values without warning Dismissed: by design; return values are harmless and not read
11 P3 failedSequences metric not incremented for sometimes test failures Dismissed: inherent to post-campaign evaluation; no worker exists to attribute the metric to

P4 — Informational

# Severity Finding Resolution
12 P4 Redundant addr := addr (Go 1.22+ loop scoping) Dismissed: pre-existing pattern across codebase
13 P4 Test uses Greater where GreaterOrEqual matches code semantics Fixed: changed to GreaterOrEqual
14 P4 Missing SPDX/pragma in Solidity test files Dismissed: informational only
15 P4 Missing StopOnFailedTest = false in TestSometimesMultipleContracts Fixed: added for consistency

Verification

  • Build: go build ./... — clean
  • Tests: TestSometimesMode and TestSometimesMultipleContracts — both pass
  • Lint: golangci-lint run --new-from-rev origin/master — 0 issues
  • Vet: go vet ./fuzzing/... — clean
  • Format: gofmt -l — clean

Commit

1b7c368 fix: resolve code review findings for PR #803

Add per-test-case locks to PropertyTestCase, AssertionTestCase, and
OptimizationTestCase, matching the pattern already applied to
SometimesTestCase. All three types had data races where Status(),
LogMessage(), and Value() read fields concurrently written by workers.

PropertyTestCase and AssertionTestCase:
  - Add lock to Status() and LogMessage()
  - Protect status/callSequence writes in FinishedCallback and
    onWorkerDeployedContractAdded

OptimizationTestCase (most severe pre-existing race):
  - Add lock to Status(), LogMessage(), and Value()
  - Protect value/callSequence comparison and update in
    callSequencePostCallTest (resolves the acknowledged TODO:
    "Should we allow for races here?")
  - Protect callSequence/trace writes in FinishedCallback
  - Protect shrinkCallSequenceRequest update
  - Value() now returns a copy to prevent callers from racing
    on the underlying big.Int

All 23 fuzzing tests pass. No lock is taken in onFuzzerStopping
since workers are already destroyed at that point.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@anishnaik

Copy link
Copy Markdown
Collaborator

Follow-up: Fix pre-existing data races in all test case providers

The race condition found in SometimesTestCase also exists in the three pre-existing test case types. Commit 4aac1bf adds per-test-case mutex protection to all of them:

Provider Fields at risk Severity
OptimizationTestCase status, value (*big.Int), callSequence, shrinkCallSequenceRequestLogMessage() mutates status during read; value comparison/update has acknowledged // TODO: Should we allow for races here? Medium
PropertyTestCase status, callSequence Low
AssertionTestCase status, callSequence Low

Changes per type

All three types:

  • Add lock sync.Mutex field
  • Status() acquires lock before reading status
  • LogMessage() acquires lock, uses t.status directly (avoids deadlock with Status())
  • FinishedCallback acquires lock around status/callSequence writes
  • onWorkerDeployedContractAdded acquires lock around NotStarted → Running transition

OptimizationTestCase additionally:

  • Value() acquires lock and returns a copy of the big.Int (prevents callers from racing on the pointer)
  • callSequencePostCallTest acquires lock for the compare-and-swap on value/callSequence (resolves the // TODO comment)
  • shrinkCallSequenceRequest update protected by lock

Verification

All 23 fuzzing tests pass (property, assertion, optimization, sometimes, chain behavior, hooks, corpus, deployment order, verbosity, filtering, external library).

Comment on lines +82 to +86
for _, contract := range t.fuzzer.ContractDefinitions() {
// If we're not testing all contracts, verify the current contract is one we specified in our target contracts.
if !t.fuzzer.config.Fuzzing.Testing.TestAllContracts && !slices.Contains(t.fuzzer.config.Fuzzing.TargetContracts, contract.Name()) {
continue
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by iterating through all the contract definitions it doesn't matter whether testAllContracts is true or false.

The best route imo is to:

  1. Iterate through targetContracts and only add test cases there
  2. Add OnFuzzWorkerDeployedContract/Deleted you should check if testAllContracts is true/false. If true, then you can add the test cases from the newly deployed contract.

There is one caveat that I don't remember is that the property test provider maintain workerState per worker while assertion test does not. I can't remember which model you should use. I think this provider is the amalgamation of both.

anishnaik and others added 3 commits April 7, 2026 13:39
- Default SometimesTesting.Enabled to false for safe upgrades
- Add "sometimes" to StopOnNoTests error messages
- Log warning when sometimes tests don't reach minExecutionCount
- Use %w for error wrapping in executeSometimesTest

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
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.

Add sometimes assertions

2 participants