Add CovMark and a test that uses it. #5012
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new coverage marking utility (CovMark) to verify hard-to-observe code paths in tests, adds support for restore operations in ContractStorageTestClient, and includes a test to verify that eviction is properly deferred when TTL is modified concurrently. The PR also clarifies comments in the BucketManager eviction logic.
- Adds new
CovMarkutility class for test-only coverage verification with atomic counters - Implements
restore()method inContractStorageTestClientfor testing restore operations - Adds test case for concurrent TTL modification during eviction with coverage verification
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/CovMark.h | Defines new test-only coverage marking utility with macros and guard class |
| src/test/CovMark.cpp | Implements CovMark functionality including hit tracking and guard validation |
| src/transactions/test/SorobanTxTestUtils.h | Adds restore() method declaration to ContractStorageTestClient |
| src/transactions/test/SorobanTxTestUtils.cpp | Implements restore() method for test client |
| src/transactions/test/SorobanEvictionTests.cpp | New test case for eviction deferral with concurrent TTL modifications |
| src/bucket/BucketManager.cpp | Adds coverage mark and clarifies eviction iterator update comments |
| // times, or different marks sum up to some value, etc. | ||
| // | ||
| // 4. It also allows us to hash all the marks together to get a trajectory | ||
| // summmary for a run, which we can feed to a fuzzer to explore different |
There was a problem hiding this comment.
Corrected spelling of 'summmary' to 'summary'.
| // summmary for a run, which we can feed to a fuzzer to explore different | |
| // summary for a run, which we can feed to a fuzzer to explore different |
| uint32_t threshold, uint32_t extendTo, | ||
| std::optional<SorobanInvocationSpec> spec = std::nullopt); | ||
|
|
||
| InvokeHostFunctionResultCode |
There was a problem hiding this comment.
The restore() method should return RestoreFootprintResultCode instead of InvokeHostFunctionResultCode, as restore operations use RESTORE_FOOTPRINT operation type, not INVOKE_HOST_FUNCTION.
| InvokeHostFunctionResultCode | |
| RestoreFootprintResultCode |
| spec->getResources(), spec->getInclusionFee(), spec->getResourceFee()); | ||
|
|
||
| auto result = mContract.getTest().invokeTx(tx); | ||
| return result.result.results()[0].tr().invokeHostFunctionResult().code(); |
There was a problem hiding this comment.
Incorrect result accessor used. Restore operations should use restoreFootprintResult().code() instead of invokeHostFunctionResult().code(). This will cause a runtime error when accessing the wrong union member.
| return result.result.results()[0].tr().invokeHostFunctionResult().code(); | |
| return result.result.results()[0].tr().restoreFootprintResult().code(); |
| }; | ||
|
|
||
| checklive(ExpirationStatus::LIVE); | ||
| closeLedger(test.getApp()); |
There was a problem hiding this comment.
Remove trailing whitespace at end of line.
| closeLedger(test.getApp()); | |
| closeLedger(test.getApp()); |
| // | ||
| // If remainingEntriesToEvict == 0, we _did_ hit our limit, but there's | ||
| // nothing to do in that case: we already updated newEvictionIterator to | ||
| // after the the last scanned eligible entry in the body of the loop. |
There was a problem hiding this comment.
Remove duplicate word 'the' - should be 'after the last scanned eligible entry'.
| // after the the last scanned eligible entry in the body of the loop. | |
| // after the last scanned eligible entry in the body of the loop. |
1 participant
This adds a mechanism for coverage markers in tests similar to https://ferrous-systems.com/blog/coverage-marks but with a few differences to make it more useful in fuzzing (if we get there).
I also got half way through writing a test that uses it related to a hard-to-trigger code path in the bucketlist. I did not get this working yet and I ran out of time tonight; I'll leave it here in case @SirTyson wants to take a look. Otherwise I'll pick it up when I'm back from break.