Skip to content

test(depgraph): pin computeLevels on a diamond and on a value cycle - #3292

Merged
thymikee merged 2 commits into
mainfrom
test/depgraph-levels
Oct 7, 2026
Merged

thymikee merged 2 commits into
mainfrom
test/depgraph-levels

Conversation

@thymikee

@thymikee thymikee commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

Summary

Follow-up to the non-blocking points in the #3275 review.

Validation

  • pnpm check:affected --run passes.
  • Iterating valueGraph.nodes instead of genPostorder makes both new tests fail.
  • The /cycle/ match in the existing cycle test fits the library's error, getTransitiveReduction: the graph contains a cycle — ….

🤖 Generated with Claude Code

View guided diff Turn on auto-fix

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues found across 1 file

View guided diff | Turn on auto-fix | Re-trigger cubic

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.12 MB 5.12 MB 0 B
Package (unpacked) 5.11 MB 5.11 MB 0 B
Package (download) 1.54 MB 1.54 MB +3 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 24.1 ms 23.8 ms -0.3 ms
CLI --help 74.4 ms 72.3 ms -2.1 ms

@thymikee

thymikee commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

This PR is ready to merge: the new computeLevels tests in scripts/depgraph/model.test.ts look correct at 0f3d17a.

I did not run the tests locally. I checked the expected diamond and cycle values by reading the code and tracing them by hand.

Not blocking: the cycle assertion (a=1, b=0) at line 306 follows the sorted node order, so starting from b would give b=1, a=0. You could assert only that every level is finite and that z is greater than a, or leave it as is.

Smoke Tests are still running. The diff only touches a depgraph unit test, which the device smoke tests do not exercise. Wait for CI to finish before merging. There are no conflicts.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 7, 2026
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@thymikee

thymikee commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

Took the cycle-order point in b76abe0, but kept the assertion exact. The two cycle members must take levels {0, 1} in either order, and z must be 2. Checking only "finite and z > a" would also pass under the old behaviour, where the closing edge added one level ({1, 2} and z = 3). Making the closing edge add a level again fails this test. pnpm check:affected --run passes.

@thymikee
thymikee merged commit e774595 into main Oct 7, 2026
19 of 21 checks passed
@thymikee
thymikee deleted the test/depgraph-levels branch October 7, 2026 18:49
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-07 18:49 UTC

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

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant