test(diamond): add comprehensive BTT coverage - #373
Conversation
👷 Deploy request for compose-diamonds pending review.Visit the deploys page to approve it
|
|
Gas ReportNo gas usage changes detected between All functions maintain the same gas costs. ✅ Last updated: Tue, 04 Aug 2026 21:09:07 GMT for commit |
…error with -ir-min
Coverage Report
Last updated: Tue, 04 Aug 2026 21:08:41 GMT for commit |
|
Thank you @megabyte0x for the amazing work! From 78% to 87%, we are getting close. We aim a full coverage of the library in the future, if you want to contribute more, you can always browse our testsuite and identify gaps. I fixed the build error with coverage. The original code created a stack too deep error when run with -ir-minimum only. I extracted the add selector looping as it's own function to reduce the variable stack. |
|
awesome! |
Summary
Adds comprehensive behavior-tree-driven tests for the Diamond module, inspect facet, and upgrade facet/module requested in issue #339.
The suite covers selector import and validation, facet linked-list bookkeeping, add/replace/remove flows, fallback forwarding and revert bubbling, upgrade events and authorization, multi-step fuzz sequences, and introspection consistency.
Closes #339.
Changes Made
test/trees/Diamond.treeas the BTT source of truth for Diamond behavior.test/.DiamondInspectFacet.facetFunctionSelectorsso a replaced facet cannot be reported through stale selector-node data.Why
DiamondInspectFacetchangedThis one-line production contract change is a regression fix revealed by the new BTT tests; it is unrelated to the coverage compiler issue.
The failing behavior was:
facetA.facetAReplacement, which exports the same selector set.facetFunctionSelectors(address(facetA)).The old implementation only checked whether the first exported selector still had any nonzero owner:
After replacement, that selector still existed but belonged to
facetAReplacement. The old check therefore returnedfacetA's selectors even thoughfacetAwas no longer registered.The updated condition verifies that the selector's current owner is the exact facet being queried:
It now returns an empty array for the replaced facet and the selector set for the active replacement, matching the function's NatSpec: “If facet is not found return empty array.” The regression is covered by
test_ShouldReturnEmptySelectors_ForFacetReplacedWithSameSelectorSet.How to Check the Changes
Run the complete contributor/CI check:
Expected result:
Run only the new Diamond suites:
Expected results:
Formatting and build can also be checked independently:
Checklist
Before submitting this PR, please ensure:
test/.forge fmtAdditional Notes
Coverage command
The repository's exact coverage command was also run:
It currently fails while compiling 419 files with Solc 0.8.35, before any test executes:
Because this is a compiler failure rather than a test failure,
--allow-failuredoes not produce a coverage summary orlcov.info. Normal builds and all 717 tests pass, including all 112 Diamond tests. Addressing the coverage compiler path would require a separate production refactor or memory-safety annotation review beyond the testing scope of issue #339.