test(flowchart): add regression coverage for numeric subgraph ids#7620
test(flowchart): add regression coverage for numeric subgraph ids#7620bayaCh05 wants to merge 44 commits intomermaid-js:developfrom
Conversation
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🦋 Changeset detectedLatest commit: cb5ae96 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
the
Type: Bug / Error
@mermaid-js/examples
mermaid
@mermaid-js/layout-elk
@mermaid-js/layout-tidy-tree
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7620 +/- ##
=======================================
Coverage 3.33% 3.33%
=======================================
Files 536 535 -1
Lines 56249 56238 -11
Branches 820 820
=======================================
Hits 1876 1876
+ Misses 54373 54362 -11
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
knsv-bot
left a comment
knsv-bot
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks @bayaCh05 — really appreciate you digging into #7609 and putting the time into writing regression tests. The jsdomIt + beforeEach wiring is clean and matches the conventions in this file nicely. 🎉
I did want to flag one finding from reviewing the PR against the original issue:
🟡 [important] — the exact bug from #7609 still reproduces on develop
The PR description says:
The reported bug suggested a Dagre-specific issue around numeric subgraph identifiers. On current
develop, these scenarios render successfully, so this PR captures that behavior with explicit tests...
This branch did not reproduce the original failure on currentdevelop; the added tests document the current expected behavior.
I ran the exact repro from #7609 against current develop:
graph LR
subgraph outer
subgraph 1 ["inner"]
external
subgraph sub
internal
end
sub-->external
end
end…and it fails with the same error reported in the issue:
TypeError: Cannot set properties of undefined (setting 'rank')
at dfs (.../dagre-d3-es/src/dagre/rank/util.js:51:24)
at longestPath (.../dagre-d3-es/src/dagre/rank/util.js:54:5)
at networkSimplex (.../dagre-d3-es/src/dagre/rank/network-simplex.js:52:3)
at rank (.../dagre-d3-es/src/dagre/rank/index.js:38:7)
...
So the bug is still present — it's just that the 4 new tests are simpler variants that don't include the specific combination that triggers it. A likely-key factor is the third-level subgraph sub containing internal, together with the cross-level edge sub-->external whose target is a sibling at the middle-subgraph scope.
Could you add one more test using the exact repro from the issue? Two outcomes, both useful:
- It passes → great, the bug really is fixed by something else, and this test is the one that will catch regression.
- It fails → this PR surfaces the bug clearly in the test suite (maybe skipped with a
.todoor clearly labeled failing test referencing #7609), which is genuinely more useful than regression coverage of adjacent scenarios.
🟡 [important] — flowchart vs graph
The issue's repro uses graph LR; the PR's tests use flowchart LR. They're aliases but not always the same code path in practice. Worth having at least one test mirror the graph keyword to match the issue's trigger exactly.
💡 [suggestion] — link the issue in the PR body
Resolves #7609 (or Refs #7609 if it shouldn't auto-close) at the top would make the provenance explicit and let GitHub cross-link them.
💡 [suggestion] — stronger assertions
svg: expect.any(String) catches "threw" vs. "didn't throw," but passes for broken-but-string SVG. For regression value, consider asserting on structural elements that would be present when the diagram actually renders correctly — e.g., the subgraph's label text or a class name from the rendered cluster. packages/mermaid/src/mermaidAPI.spec.ts has other tests that do shallow string checks on the SVG you could mirror.
Summary: the groundwork here is solid, the naming and structure are consistent with the rest of the suite, and the alphabetic control case (test 4) is a nice touch. Just needs one more test that reflects the actual reported failure so this PR lands the regression coverage it's aiming for. Happy to pair on narrowing down the minimal failing repro if that helps.
|
@knsv-bot I’m going to keep that regression test in place and investigate a scoped fix on this same branch, so the PR can cover both the failing case and the actual resolution. |
|
Thanks again for the review — I added the exact repro from #7609 and traced the failure into the Dagre cluster-adjustment path. The root cause was that cluster-edge adjustment did not treat edges connected directly to a cluster node as external connections in this shape, so the edge could remain attached to the cluster instead of being rewritten to a valid descendant anchor. I’ve now updated that logic, kept the exact regression test, and added a focused lower-level test to lock in the edge-rewrite behavior directly. |
The tidy-tree layout's calculateEdgePositions only added intermediate routing points for source/target nodes in the 'left' or 'right' section. For root-sourced edges (section === 'root'), no intermediate point was pushed, so the post-loop intersection recompute used the child's center as the reference and could land the start anchor on the root's top/bottom edge instead of its left/right edge — visually disconnecting the link. The cloud root only appeared correct by coincidence of its rounded shape. Add 'root' branches to both the source and target intermediate-point blocks. The root-side intermediate is placed on the side facing the other node's section. Resolves mermaid-js#7572 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Restore and properly implement nested namespaces, which regressed
between 11.3.0 and 11.4.x. Both dot notation (namespace A.B.C) and
syntactic nesting (namespace A { namespace B {} }) now create
hierarchical namespace clusters in the rendered diagram.
Closes mermaid-js#3384, mermaid-js#4618, mermaid-js#5487, mermaid-js#6085
Co-Authored-By: Claude Opus 4.6 (prompted with care by @M-a-c)
Co-Authored-By: Claude Opus 4.6 (prompted with care by @M-a-c)
For syntactic nesting (namespace A { namespace B {} }), the displayed
label now shows the short name "B" instead of the qualified "A.B".
Each namespace stores a separate label (last segment of the id) for
display, while the full dot-separated id is used internally for
graph wiring and uniqueness.
Co-Authored-By: Claude Opus 4.6 (prompted with care by @M-a-c)
Add square bracket label syntax for namespaces, matching the existing
class label pattern: namespace Auth["Authentication Service"] { }
The label replaces the displayed name while the id is used internally.
Closes mermaid-js#6018
Co-Authored-By: Claude Opus 4.6 (prompted with care by @M-a-c)
Add visual regression tests across all four renderers (v2, v3, ELK, handDrawn) covering dot-notation nesting, syntactic nesting, and labeled namespaces. Existing namespace tests will produce updated snapshots due to the new hierarchical cluster rendering. Co-Authored-By: Claude Opus 4.6 (prompted with care by @M-a-c)
Co-Authored-By: Claude Opus 4.6 (prompted with care by @M-a-c)
When set to false, only user-declared namespaces render as flat boxes using their full qualified name; auto-created intermediate ancestors are elided and their children moved to the nearest explicit ancestor. Defaults to true (existing nested behavior). Adds explicit field to NamespaceNode, updates both v2 and v3 renderers, demos, docs, unit tests, and Cypress E2E snapshots. Co-Authored-By: Claude Opus 4.6 (prompted with care by @M-a-c)
…ple times Fixes mermaid-js#7560 where cardinality labels (e.g. "1", "0..1") were displayed 3x on self-referential class diagram relationships. Root cause: The dagre layout splits self-loops into 3 edges but structuredClone copied cardinality labels to all of them. Now each segment only carries its relevant cardinality label. Also fix DOM hierarchy bug in edge label creation where labels were appended to the wrong parent element. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The middle edge segment of a self-loop should preserve its label (e.g. "refers") — only the cardinality labels need to be cleared. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Clear label props on split sub-edges to prevent multiplicity labels from rendering 3x after structuredClone during layout. - keep labels only on correct sub-edges - defensively clear all label positions - remove unintended arrow on edge2 - add visual regression test - add changeset
Require a newline boundary before end note so it is only matched when used as the actual note terminator. Previously, the parser could terminate a note when it appeared inside text (e.g., "send note"). Also, adds a regression test for this scenario. Fixes mermaid-js#7089
Address review feedback by: -adding assertions for note content, not just state relations; -splitting test cases to separately cover two edge cases: "send note" and "end note" inside note text. Fixes mermaid-js#7089
… Langium validator Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Lines starting with %% are now only recognized as comments if they are on their own line. Inline %% comments or lines starting with a single % are no longer treated as comments and are parsed as normal states or transitions. Previously, the parser could: - Treat %% inline comments as actual comments, losing line content. - Treat lines starting with % as comments, discarding valid states. Adds regression tests for each scenario. Fixes mermaid-js#7090
Addresses feedback from the review: - Comments after blank lines are now recognized; - Adds tests for blank-line and edge-case scenarios; - Adds a brief code comment in the test explaining the expected behavior for inline %%. Fixes mermaid-js#7090
Addresses feedback from the review: - Updates lexer to allow '%%' comments both at start-of-line and inline; - Treats a single '%' as normal text instead of a comment; - Updates stateDiagram.md documentation to clarify the new comment syntax; - Adds unit tests for inline comments and single '%' scenarios. Fixes mermaid-js#7090
Move all diagram-specific Cypress specs from the flat cypress/integration/rendering/ directory into per-diagram subfolders (e.g. cypress/integration/rendering/flowchart/). The detection script now uses filesystem discovery instead of a hardcoded DIAGRAM_SPEC_MAP: it checks whether cypress/integration/rendering/<diagram-name>/ exists and returns a glob pattern (cypress/integration/rendering/<name>/**) as the --spec argument. Adding a new spec to a subfolder requires zero config changes. Cross-cutting specs (theme, conf-and-directives, shapes, etc.) remain at the root of cypress/integration/rendering/ and continue to trigger the full suite. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ntion Any spec file at the root of cypress/integration/rendering/ is treated as cross-cutting (full suite). Any spec in a subfolder is scoped to that subfolder. No explicit list to maintain — the directory position is the convention. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
❌ Lockfile Validation Failed The following issue(s) were detected: Please address these and push an update. Posted automatically by GitHub Actions |
|
Thanks for tracing this into the cluster-edge adjustment, @bayaCh05 — the reasoning is sound. That said, the Argos build is showing some unexpected cluster-related visual regressions that look tied to this change — see build 6655. There are cluster layout shifts in subgraph rendering that don't look intentional; it seems the edge-rewrite is affecting more shapes than just the failing numeric-id case. Could you take a pass through those diffs and narrow down which branch of the rewrite logic is firing on previously-valid inputs? We'll need those resolved before this is mergeable. Happy to help dig if you get stuck. |
10 participants
Summary
Adds focused regression coverage for flowchart subgraphs that use numeric identifiers in the Dagre render path.
What changed
Why
The reported bug suggested a Dagre-specific issue around numeric subgraph identifiers. On current
develop, these scenarios render successfully, so this PR captures that behavior with explicit tests and helps guard against regressions.Verification
pnpm exec vitest run packages/mermaid/src/mermaidAPI.spec.ts -t "flowchart numeric subgraph ids"Notes
This branch did not reproduce the original failure on current
develop; the added tests document the current expected behavior.