Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

E2E testing isn't working for PR #5152 failing due to randomized commit ID #5343

Closed
rowanfr opened this issue Feb 29, 2024 · 2 comments · Fixed by #5344
Closed

E2E testing isn't working for PR #5152 failing due to randomized commit ID #5343

rowanfr opened this issue Feb 29, 2024 · 2 comments · Fixed by #5344
Labels
Status: Triage Needs to be verified, categorized, etc Type: Bug / Error Something isn't working or is incorrect

Comments

@rowanfr
Copy link
Contributor

rowanfr commented Feb 29, 2024

Description

The bug repeatedly cause E2E failures in testing.

Steps to reproduce

This is reproduced every time a commit is pushed in mermaid during the E2E cypress testing in cypress/integration/rendering/gitGraph.spec.js

Screenshots

Git-Graph-diagram-46-should-render-GitGraph-with-merge-back-and-merge-forward diff
![Git-Graph-diagram-47-should-render-GitGraph-with-merge-back-and-merge-forward--Vertical-Branch diff](https://github.c
Git-Graph-diagram-48-should-render-GitGraph-with-merge-on-a-new-branch--Vertical-Branch diff
om/mermaid-js/mermaid/assets/28838223/a47fcc20-4c18-46e2-9b47-d04825775387)
Git-Graph-diagram-49-should-render-GitGraph-with-merge-on-a-new-branch--Vertical-Branch diff

Code Sample

it('46: should render GitGraph with merge back and merge forward', () => {
    imgSnapshotTest(
      `gitGraph LR:
      commit
      branch branch-A
      branch branch-B
      commit
      checkout branch-A
      merge branch-B
      checkout branch-B
      merge branch-A
      `,
      { gitGraph: { parallelCommits: true } }
    );

Setup

  • Mermaid version: Latest, 1857eb1
  • Browser and Version: Chrome

Suggested Solutions

Add specific commit id's similar to the other git graph tests for consistency.

Additionally we should likely check-in the test images in the actual repository itself to have some base truth for testing that is modifiable rather than relying upon the most recently cached image (as best as I understand the E2E testing). Currently because of that this test will fail despite modification to it to resolve random titles as the previous image is cached leading to either a desire to change the name of the tests or merge without successful E2E testing, both of which are bandaid solutions to a lack of modifiable base truth for E2E testing.

@rowanfr rowanfr added Status: Triage Needs to be verified, categorized, etc Type: Bug / Error Something isn't working or is incorrect labels Feb 29, 2024
rowanfr added a commit to rowanfr/mermaid that referenced this issue Feb 29, 2024
@sidharthv96
Copy link
Member

Additionally we should likely check-in the test images in the actual repository itself to have some base truth for testing that is modifiable rather than relying upon the most recently cached image (as best as I understand the E2E testing)

We have an applitools test suite which we use before every release. But they have a limited volume for OSS projects and we don't want to run it on every test. The current option is a tradeoff so that we have some checks on all PRs, rather than get multiple failures right before we want to make a release. In this case, we'll know what's failing before merging a PR.

I understand this is not the best scenario, and is a bandaid, but it prevents a bigger pain for maintainers down the line.
We're very much open for better solutions :)

sidharthv96 added a commit that referenced this issue Feb 29, 2024
@rowanfr
Copy link
Contributor Author

rowanfr commented Feb 29, 2024

@sidharthv96 Thank you for explaining the reasoning behind why this is the current circumstance, it makes far more sense knowing that. While I'd love to provide an easier solution, I think the one currently being used was correctly identified as the most reasonable one for maintainers presently

sidharthv96 added a commit that referenced this issue Mar 1, 2024
* develop:
  Resolves E2E testing issues and issue #5343
sidharthv96 added a commit to Yokozuna59/mermaid that referenced this issue Mar 1, 2024
* next: (100 commits)
  Resolves E2E testing issues and issue mermaid-js#5343
  Fix spelling
  Fix community integrations
  Fix docs
  docs: Fix config
  Update all minor dependencies
  Amend docs to document gitgraph parallel commits
  Fix lint
  Use Yarn Add COREPACK_ENABLE_STRICT
  Added link to Blazorade Mermaid to the community integrations page.
  Bump node version
  Add lcov to cspell
  Correcting path to docker-entrypoint.sh
  Update recommended Vitest extension
  Replace mermaid-js.github.io links
  Replace links to docs with links to webhelp
  Link to contributing page on webhelp
  Changes to timeline.md 1. Added colons to all 'NOTES' for consistency.
  Changes to timeline.md 1. Updates the Wikipedia citation to include a link. 2. Removed periods from documentation sections to be consistent (some had periods, some didn't) 3. Added a space to a coding example for spacing consistency.
  Replace version number placeholder
  ...
sidharthv96 added a commit that referenced this issue Mar 1, 2024
* next: (269 commits)
  Resolves E2E testing issues and issue #5343
  Fix spelling
  Fix community integrations
  Fix docs
  docs: Fix config
  Update all minor dependencies
  Amend docs to document gitgraph parallel commits
  Fix lint
  Use Yarn Add COREPACK_ENABLE_STRICT
  Added link to Blazorade Mermaid to the community integrations page.
  Bump node version
  Add lcov to cspell
  Correcting path to docker-entrypoint.sh
  Update recommended Vitest extension
  Replace mermaid-js.github.io links
  Replace links to docs with links to webhelp
  Link to contributing page on webhelp
  Changes to timeline.md 1. Added colons to all 'NOTES' for consistency.
  Changes to timeline.md 1. Updates the Wikipedia citation to include a link. 2. Removed periods from documentation sections to be consistent (some had periods, some didn't) 3. Added a space to a coding example for spacing consistency.
  Replace version number placeholder
  ...
sidharthv96 added a commit that referenced this issue Mar 5, 2024
* develop:
  Update docs
  Lychee ignore chrome webstore
  Update link
  chore(deps): update all patch dependencies
  build(docs): vendor CSS dependencies
  chore(deps): update all minor dependencies
  Ran lint:fix
  Fix chrome webstore url causing 404
  Resolves E2E testing issues and issue #5343
sidharthv96 added a commit that referenced this issue Mar 6, 2024
* develop: (280 commits)
  chore: Remove unused imports in block
  Fix spelling
  Update docs
  Lychee ignore chrome webstore
  Update link
  chore(deps): update all patch dependencies
  build(docs): vendor CSS dependencies
  chore(deps): update all minor dependencies
  Ran lint:fix
  Fix chrome webstore url causing 404
  build(deps): update `langium` to `v3` and apply the required changes
  Resolves E2E testing issues and issue #5343
  Fix spelling
  Fix community integrations
  Fix docs
  docs: Fix config
  Update all minor dependencies
  Amend docs to document gitgraph parallel commits
  Fix lint
  Use Yarn Add COREPACK_ENABLE_STRICT
  ...
sidharthv96 added a commit that referenced this issue Mar 6, 2024
* develop: (280 commits)
  chore: Remove unused imports in block
  Fix spelling
  Update docs
  Lychee ignore chrome webstore
  Update link
  chore(deps): update all patch dependencies
  build(docs): vendor CSS dependencies
  chore(deps): update all minor dependencies
  Ran lint:fix
  Fix chrome webstore url causing 404
  build(deps): update `langium` to `v3` and apply the required changes
  Resolves E2E testing issues and issue #5343
  Fix spelling
  Fix community integrations
  Fix docs
  docs: Fix config
  Update all minor dependencies
  Amend docs to document gitgraph parallel commits
  Fix lint
  Use Yarn Add COREPACK_ENABLE_STRICT
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Triage Needs to be verified, categorized, etc Type: Bug / Error Something isn't working or is incorrect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants