Fixes support of the macro ContainerQueue_Ext for C4 diagrams definition.

[kislerdm]

📑 Summary

Fixes support of the macro ContainerQueue_Ext for C4 diagrams definition.

Chores

• Trailing spaces removed
• Test covering the Container... macros added

Resolves #4576

📏 Design Decisions

NA

📋 Tasks

• 📖 have read the contribution guidelines
• 💻 have added unit/e2e tests (if appropriate)
  📓 have added documentation (if appropriate)
• 🔖 targeted develop branch

[codecov]

Codecov Report

Merging #4577 (584b51d) into develop (0a493c7) will increase coverage by 31.70%.
The diff coverage is n/a.

Additional details and impacted files

@@             Coverage Diff              @@
##           develop    #4577       +/-   ##
============================================
+ Coverage    45.28%   76.99%   +31.70%     
============================================
  Files           52      143       +91     
  Lines         6633    14455     +7822     
  Branches        18      516      +498     
============================================
+ Hits          3004    11130     +8126     
+ Misses        3629     3220      -409     
- Partials         0      105      +105     

Flag	Coverage Δ
e2e	83.95% <ø> (?)
unit	45.28% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 137 files with indirect coverage changes

[sidharthv96]

I'm not familiar with C4. But the changes looks good to me.
Can you add some imgSnapshotTests too?

[aloisklink]

These changes look great to me too! Thanks for adding some tests as well.

IMO, for other reviewers, the only actual changes are in these lines of JISON: 592e9c9#diff-ebeabc10483d769581539e8515fcc9851825401a6ffce06e48e1114b80c68b73R170-R173

Btw, @kislerdm, it doesn't matter too much since this PR is pretty small (and Mermaid is pretty lax about commits), but it does make reviewers life easier if refactoring changes (like removing trailing spaces) are in a separate commit from bug fixes. Other than that, this PR was pretty much perfect 😄

---

I'm not familiar with C4

I'm also not too familiar with C4 diagrams. @pinghe is the one who added them originally, so it might be worth waiting for a few days in case they want to give a review, but IMO, this fix is pretty small, so it should be fine!

[sidharthv96]

@aloisklink this setting was helpful for me :)

[aloisklink]

@aloisklink this setting was helpful for me :)

👍 Very cool, I didn't know GitHub added that feature! I've always used git diff -w since I thought GitHub didn't have that feature!

IMO, I'll probably still have to leave it off by default. I'm often looking at Python code, where whitespace matters, and GNU Makefiles, where tabs (\t) and spaces both do different things.

[kislerdm]

@sidharthv96 @aloisklink Hey folks! Thanks a lot for your review.

@aloisklink, you are right that the only tangible change this PR contains is the update of the JISON file :) However, I believe in "small" refactoring, hence I'd rather include trimmed spaces to PR as well if you don't mind.

@sidharthv96 could you please help me by expanding on your question? Thanks!

Can you add some imgSnapshotTests too?

It'd enable me to execute on your suggestion.

[aloisklink]

However, I believe in "small" refactoring, hence I'd rather include trimmed spaces to PR as well if you don't mind.

No worries, that's fine! It would have been a bit nicer to have the trimmed spaces as a separate commit in this PR, but again, it's such a small change that it doesn't really matter 😄 The mermaid project doesn't have strict commit guidelines.

Can you add some imgSnapshotTests too?

I think @sidharthv96 would like an E2E cypress test that uses ContainerQueue_Ext in an imgSnapshotTest() in: https://github.com/mermaid-js/mermaid/blob/develop/cypress/integration/rendering/c4.spec.js

You can run these tests with pnpm run e2e, but they are very slow, since they run on an actual browser. Personally, since your PR is quite a small change, and already well tested, I don't think adding an imgSnapshotTest is necessary, but it would still be nice to have one :)

[kislerdm]

@aloisklink Thank you for clarification! I feel that the added unit tests are sufficient for the change.

Moreover, reasonable e2e testing would require a set of decisions:

• What is the focus of the e2e test? Happy path only?
• What shall be used as a reference?
• Shall the "non external" container's colour be compared to the colour of an "external" container? For context, the external is grey, non-external is blue.

[kislerdm]

@aloisklink @sidharthv96 @knsv Hey folks! Would it possible to include the fix to upcoming release?

[aloisklink]

Hey folks! Would it possible to include the fix to upcoming release?

Generally, Knut needs to review anything that changes a JISON file, but I think since the actual non-whitespace change is pretty minor, it might be okay to merge this without their approval. @sidharthv96, are we okay to merge without a review from @knsv?

[sidharthv96]

Knut is on vacation, so we can go ahead.

[mermaid-bot]

@kislerdm, Thank you for the contribution!
You are now eligible for a year of Premium account on MermaidChart.
Sign up with your GitHub account to activate.