test(sankey): add tests for special characters in node names#7545
Conversation
Add explicit test cases for special characters (single quotes, ampersands, forward slashes, and hyphens) in Sankey diagram node names. These tests verify the fix for issue mermaid-js#7528 where special characters in node names like 'Agricultural \'waste\'', 'Lighting & appliances', and 'Over generation / exports' are correctly parsed. The tests confirm that both 'sankey' and 'sankey-beta' syntax properly handle these characters in CSV-style diagram definitions.
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@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 #7545 +/- ##
==========================================
- Coverage 3.31% 3.31% -0.01%
==========================================
Files 539 540 +1
Lines 56719 56730 +11
Branches 824 824
==========================================
Hits 1880 1880
- Misses 54839 54850 +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 ↗︎ Awaiting the start of a new Argos build… |
knsv
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for taking the time to add test coverage for special character handling in Sankey diagrams, @cyphercodes! Documenting parser behavior with explicit tests is valuable, and the test cases you've written are well-structured with good assertions on both node and link counts. 🎉
What's working well
🎉 [praise] The two new test cases cover a meaningful set of special characters (single quotes, ampersands, forward slashes, hyphens) that match exactly what was reported in #7528. The assertions on graph.nodes and graph.links counts verify the parser actually processed the data, not just that it didn't throw. Nice work.
🎉 [praise] Testing both sankey-beta and sankey syntax variants is thorough and consistent with the existing test pattern in the file.
Things to address
🔴 [blocking] Unrelated markers.js change must be in a separate PR
markers.js:21-22 — This PR changes the extensionStart marker dimensions from 190×240 → 20×28. This change:
- Is completely unrelated to Sankey special character parsing and is not mentioned anywhere in the PR description.
- Modifies shared rendering code (
rendering-util/) — this marker is used by class diagrams for inheritance arrows. Changes here can visually regress any diagram type that uses theextensionmarker. - Has no tests or visual verification. Changes to shared rendering code require E2E visual regression tests to confirm no diagrams are broken.
While the old values (190×240) do look anomalously large compared to the matching extensionEnd marker (20×28) and the -margin variants, this may be intentional for the userSpaceOnUse coordinate system on that specific marker. Either way, it needs its own PR with:
- An explanation of why the change is needed
- Before/after screenshots of class diagrams showing inheritance arrows
- E2E test coverage
Please revert the markers.js change from this PR. If you believe it's a legitimate fix, it would be great to open a separate PR for it with visual evidence.
🟡 [important] PR doesn't actually fix #7528
The PR body states: "The tests confirm that the parser already correctly handles these characters. The issue reported in #7528 appears to have been resolved in the current version."
This means the PR documents existing behavior rather than fixing a bug. That's still useful, but:
- The PR title says
test(sankey)which is accurate — but theFixes #7528in the body will auto-close the issue on merge. If the parser already works, the issue may have been about a different version or a different root cause (the issue mentions v11.12.1). It would be worth confirming whether the original reporter's exact input from the issue still fails, or whether this was fixed between v11.12.1 and the current version. If it was already fixed, a comment on #7528 explaining which version resolved it would be more appropriate thanFixes.
🟢 [nit] Missing changeset
Since this is test-only (no user-facing change), a changeset isn't strictly required. But if the markers.js change were included, it would definitely need one. Just noting for awareness.
Security
No XSS or injection issues identified. The changes are test-only (for the Sankey portion) and static numeric attribute values (for the markers portion). No user-controlled input flows through the changed code paths.
Self-check
- At least one 🎉 [praise] item exists
- No duplicate comments
- Severity tally: 1 🔴 blocking / 1 🟡 important / 1 🟢 nit / 0 💡 suggestion / 2 🎉 praise
- Verdict matches criteria: REQUEST_CHANGES (1 🔴)
- Not a draft PR — REQUEST_CHANGES is appropriate
- No inline comments used
- Tone check: collaborative and appreciative ✓
|
Updated this PR to address the review feedback:
CI status after the push: unit tests, lint, docs, CodeQL, Netlify, and other non-E2E checks are passing. The E2E shards are currently failing during Argos upload with:
That appears to be an Argos/account quota issue rather than a Sankey test failure. |
|
Updated this PR with a narrower follow-up for the remaining #7528 coverage concern:
Commit: Local verification:
CI after the push: unit test, lint, build-docs, CodeQL, Netlify preview, dependency review, docker-lint, preview release, and autofix are passing. The only failures are the 5 E2E shards, all failing during Argos upload with:
That appears to be an Argos/account quota blocker rather than a failure caused by this test-only Sankey change, so I did not make any workflow or Argos bypass changes. |
|
Thanks @cyphercodes, I will approve and merge your PR. I can confirm, the e2e failing is not related to your PR. |
Summary
This PR adds explicit regression coverage for special characters in Sankey diagram node names using the exact "FY20-21 Performance" sample reported in #7528.
Related Issue
Refs #7528 - Sankey Diagram Parsing Failure: Special Characters in Node Names Trigger "Syntax error in text"
Changes
sankey.spec.tsfor the reported sample with bothsankey-betaandsankeysyntax.Test Coverage
The tests verify that the following special characters are correctly parsed in node names:
Agricultural 'waste'Lighting & appliances - commercial,Lighting & appliances - homesOver generation / exportsHeating and cooling - homes,Heating and cooling - commercialVerification
The tests confirm that the parser currently handles the original reported Sankey input. This PR adds regression coverage for the reported character set without auto-closing #7528.
Checklist