Skip to content

test(diamond): add comprehensive BTT coverage - #373

Merged
maxnorm merged 13 commits into
Perfect-Abstractions:mainfrom
megabyte0x:issue-339-diamond-btt-tests
Aug 4, 2026
Merged

test(diamond): add comprehensive BTT coverage#373
maxnorm merged 13 commits into
Perfect-Abstractions:mainfrom
megabyte0x:issue-339-diamond-btt-tests

Conversation

@megabyte0x

@megabyte0x megabyte0x commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

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

  • Added test/trees/Diamond.tree as the BTT source of truth for Diamond behavior.
  • Added shared Diamond fixtures, harnesses, mocks, and storage readers under test/.
  • Added unit coverage for selector import, facet addition, fallback dispatch, and upgrade behavior through both facet and module surfaces.
  • Added integration coverage for inspection and stateful add/replace/remove sequences.
  • Added fuzz/property checks for selector and facet counts, list-link integrity, delegatecall behavior, events, authorization, and introspection after upgrades.
  • Fixed DiamondInspectFacet.facetFunctionSelectors so a replaced facet cannot be reported through stale selector-node data.

Why DiamondInspectFacet changed

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

  1. Add facetA.
  2. Replace it with facetAReplacement, which exports the same selector set.
  3. Call facetFunctionSelectors(address(facetA)).

The old implementation only checked whether the first exported selector still had any nonzero owner:

s.facetNodes[facetSelectors[0]].facet == address(0)

After replacement, that selector still existed but belonged to facetAReplacement. The old check therefore returned facetA's selectors even though facetA was no longer registered.

The updated condition verifies that the selector's current owner is the exact facet being queried:

s.facetNodes[facetSelectors[0]].facet != _facet

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:

FOUNDRY_PROFILE=ci npm run compose@check

Expected result:

Ran 166 test suites: 717 tests passed, 0 failed, 0 skipped

Run only the new Diamond suites:

FOUNDRY_PROFILE=ci forge test --match-path "test/unit/diamond/**" -vvv
FOUNDRY_PROFILE=ci forge test --match-path "test/integration/diamond/**" -vvv

Expected results:

  • Diamond unit tests: 97 passed
  • Diamond integration tests: 15 passed
  • Fuzz tests use 1,000 runs under the CI profile

Formatting and build can also be checked independently:

forge fmt --check
FOUNDRY_PROFILE=ci forge build --sizes

Checklist

Before submitting this PR, please ensure:

  • Code follows the Solidity feature ban - The production change introduces no banned Solidity features; test helpers remain under test/.
  • Code follows Design Principles - Uses existing diamond storage and composition patterns.
  • Code matches the codebase style - Follows the existing BTT, fixture, fuzz-test, and naming conventions.
  • Code is formatted with forge fmt
  • Existing tests pass - The full suite passes.
  • New tests are optional - This PR adds the tests requested by issue [TESTING] Add BTT tests for diamond module, inspect & upgrade facets/modules #339.
  • All tests pass - 717/717 passed under the CI profile.
  • Documentation updated - Added the Diamond behavior tree specification.
  • Changesets - Reviewed; no changeset is included, and maintainers can advise whether the introspection fix needs one.

Additional Notes

Coverage command

The repository's exact coverage command was also run:

FOUNDRY_PROFILE=ci forge coverage --ir-minimum --allow-failure --report summary --report lcov

It currently fails while compiling 419 files with Solc 0.8.35, before any test executes:

Error: Variable expr_9 is 1 too deep in the stack [...]
No memoryguard was present. Consider using memory-safe assembly only and annotating it via 'assembly ("memory-safe") { ... }'.

Because this is a compiler failure rather than a test failure, --allow-failure does not produce a coverage summary or lcov.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.

@netlify

netlify Bot commented Aug 4, 2026

Copy link
Copy Markdown

👷 Deploy request for compose-diamonds pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit c28e9b3

@changeset-bot

changeset-bot Bot commented Aug 4, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: c28e9b3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@maxnorm
maxnorm requested a review from mudgen August 4, 2026 19:30
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Gas Report

No gas usage changes detected between main and issue-339-diamond-btt-tests.

All functions maintain the same gas costs. ✅

Last updated: Tue, 04 Aug 2026 21:09:07 GMT for commit c28e9b3

@maxnorm
maxnorm removed the request for review from mudgen August 4, 2026 20:23
@maxnorm
maxnorm marked this pull request as ready for review August 4, 2026 21:06
@maxnorm maxnorm added the Testing label Aug 4, 2026
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Coverage Report

Coverage

Metric Coverage Details
Lines 87% 1965/2268 lines
Functions 91% 533/585 functions
Branches 93% 201/216 branches

Last updated: Tue, 04 Aug 2026 21:08:41 GMT for commit c28e9b3

@maxnorm

maxnorm commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

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.

@maxnorm
maxnorm merged commit b42e98b into Perfect-Abstractions:main Aug 4, 2026
5 checks passed
@megabyte0x

Copy link
Copy Markdown
Contributor Author

awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[TESTING] Add BTT tests for diamond module, inspect & upgrade facets/modules

2 participants