test: improve test usefulness and fuzz coverage - #4341
Merged
Merged
Conversation
The round-trip helper's error was discarded at 38 call sites, which hid three fuzz generators that could never produce a value the marshaler accepts. fuzzEphemeralPublicKey fuzzed X and Y directly, so the point was almost never on secp256k1 and SerializeCompressed emitted keys ParsePubKey rejects. fuzzEphemeralPrivateKey fuzzed the public half and D independently, so the two halves never agreed and IsKeyMatching would reject every key it made. The tbtc DepositSweepProposal generator produced unbounded reveal blocks where Marshal requires IsUint64. Assert the discarded errors, derive both key halves from one normalized scalar, and clamp reveal blocks in a local generator rather than narrowing the shared big.Int generator. Rename FuzzUnmarshaler to AssertUnmarshalDoesNotPanic so its name matches the only thing it checks.
shouldSkipElectrumIntegrationError matched three substrings of err.Error(), so rewording an upstream message silently turned a skipped integration test into a passing one, and an unrelated error whose text merely mentioned a timeout was skipped. Match errors.Is against goelectrum.ErrTimeout, wrappers.ErrRetryTimeout and electrum.ErrFeeEstimateUnavailable. Document both new sentinels on the variable and on the function that returns them, so depending on their identity is a stated promise rather than an accident. Regression tests cover bare, wrapped and doubly wrapped sentinels plus the two negative cases that guard against over-skipping.
Two electrum tests ranged over a fixture map with no presence check, so deleting a fixture made them run zero subtests and pass. The scenario loaders behave the same way: filepath.Walk skips absent files and returns a shorter slice with a nil error, so a consuming test loops zero times and reports success. Add an explicit manifest of required fixtures per directory and a guard that fails when one is absent. Each guard lives in the consuming package, never under testdata, because go test excludes that directory from ./... and a guard placed there would never run.
Several tests passed regardless of the behavior in their own title. TestContextCancelation claimed to verify that goroutines stop on cancellation but only called three methods and asserted nothing; it now runs observeSystemMetrics directly and fails if it does not return. TestLocalSubmitRelayEntry checked only that no error came back, so an implementation that validated its input and discarded it would pass; it now asserts the entry is readable afterwards. Replace 22 sleeps in the scheduler tests with channel synchronization and bounded polling that fails on timeout, cutting the package from 4.6s to 0.8s. Keep a real observation window where the assertion is that nothing happens, since proving a negative needs one. Split the two longest tests, 683 and 677 lines against a repo median of 33, into behavior-named functions with the same case counts.
The repo had no func FuzzXxx(f *testing.F) targets, so every fuzzed input came from a fixed generator and nothing was kept between runs. Add round-trip targets for the two paths whose generators were found broken: ephemeral key marshaling and the deposit sweep proposal. Both assert equality after the round trip rather than mere absence of a panic, which is what makes them able to catch the original bugs. Seeds cover the empty input, small values and the top of each accepted domain.
The two authorization suites each repeated the same two test bodies 25 times under different describe contexts. The repetition was contextually correct, but fixing one assertion meant editing it 14 times. Move the shared bodies into behavior functions invoked per context, passing the fixture through a getter so each test reads the instance created by its own beforeEach rather than a stale reference captured at module load. Test counts are unchanged at 118 and 144.
…appened assertNoValueChange treated any send on tw.iterated as proof the worker had run after being stopped. workerFunc increments the counter and sends on the channel as two separate steps, so a worker that incremented before the stop can deliver its signal after it. At -count=20 this failed while reporting an identical value on both sides of the comparison, which is a false positive. Use the counter as the source of truth and treat the signal only as a reason to re-check early. The package now passes at -count=20 in 16.8s against 30.7s for the original sleep-based version, and at -count=50. Also correct both native fuzz target comments. They claimed to catch the generator bugs that motivated them, but each target builds its own input and never calls FuzzFuncs, so neither would fail if a generator regressed. The comments now name the tests that do guard the generators and state what the targets actually cover.
The `describe.skip` on "when a contract gets upgraded during DKG" carried a two-part rationale and both parts are now stale. The fixture does accept `useAllowlist: false` as an explicit option, and the packaged TokenStaking artifacts do still expose `stake` and `increaseAuthorization`. Removing the skip restores "keeps data of the existing wallet" and "stores data of a new wallet". Both fail when mutated: swapping the expected public key and the expected members hash takes the file from 19 passing to 17 passing and 2 failing. The ecdsa suite is 836 passing with no pending tests and no remaining skips.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Keep Testnet fixtures required through the default manifest test while allowing the integration suite to skip Mainnet and Testnet4 when those networks have no vectors. Restore the justified G404 suppression for non-security backoff jitter.
Align the new and changed comments with the code they describe: - internal/testdata/bitcoin/manifest.go: the integration tests now skip (presence check) when a network has no Transactions entry, so the RequiredTransactions doc no longer claims they range the map without a check - pkg/bitcoin/electrum/electrum.go: EstimateSatPerVByteFee and ErrFeeEstimateUnavailable docs now state that the returned error wraps the last target's error, so the sentinel matches only when the last failure was an oracle -1 - solidity/ecdsa/test/Allowlist.test.ts: the two reverting cases are in the initialization group, not below it - drop previously/now diff narration from the wrappers, generator, pbutils, and electrum skip-classifier comments, keeping the durable invariants they document
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
go test ./...Follow-up test-usefulness work for the
devrelease branch.