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.

Sign up for GitHub

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

Jump to bottom

chore: fix e2e #2961

Closed
wants to merge 8 commits into from
Closed

chore: fix e2e #2961

wants to merge 8 commits into from

Conversation

Yash-Singh1
Copy link
Member

@Yash-Singh1 Yash-Singh1 commented Apr 22, 2022 •
edited
Loading

📑 Summary

Fixes the e2e problems with the image snapshots and prevents double running workflows on PRs.

📏 Design Decisions

Describe the way your implementation works or what design decisions you made if applicable.

Moved to cypress-plugin-snapshots instead of the image snapshots plugin.

📋 Tasks

Make sure you

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

@Yash-Singh1 Yash-Singh1 requested a review from knsv April 22, 2022 23:34
@knsv
Copy link
Collaborator

knsv commented Apr 28, 2022

Thais one fixes the e2e tests but in the process it also removes the image snapshot tests which is why I will not merge this. Thank you nevertheless.

@knsv knsv closed this Apr 28, 2022
@Yash-Singh1
Copy link
Member Author

@knsv This doesn’t remove the image snapshots. It just uses a different dependency to record image snapshots.

@Yash-Singh1 Yash-Singh1 reopened this Apr 28, 2022
@Yash-Singh1
Copy link
Member Author

The root issue was: cypress-io/cypress#3090.

@github-actions
Copy link
Contributor

🌀 Tests overview by Testomatio

Found 341 cypress tests in 23 files

✔️ Added 2 tests

+ Flowchart: 34: testing the label width
+ Graph: 34: testing the label width

🗑️ Removed 2 tests

- Flowchart: 34: testing the label width in percy
- Graph: 34: testing the label width in percy
📎 List all suites (24)

@sidharthv96
Copy link
Member

@Yash-Singh1 cypress has been upgraded. So you'll have to move the changes in cypress.json to cypress.config.js.

@sidharthv96
Copy link
Member

When running the branch in my machine, the following screenshots had diff and 2 other tests failed.

image

image

image

image

I'm wondering why this happened to just these 2 diagrams.

 1) Sequence diagram
       svg size
         should render a sequence diagram when useMaxWidth is true (default):
     AssertionError: Timed out retrying after 4000ms: expected 970 to be within 920..960
      at Context.eval (http://localhost:9000/__cypress/tests?p=cypress/integration/rendering/sequencediagram.spec.js:2912:30)

  2) Sequence diagram
       svg size
         should render a sequence diagram when useMaxWidth is false:
     AssertionError: Timed out retrying after 4000ms: expected 970 to be within 920..960
      at Context.eval (http://localhost:9000/__cypress/tests?p=cypress/integration/rendering/sequencediagram.spec.js:2952:30)

This might be a config issue with macbook screen resolution. We could either increase the range to 970 or find some way to limit browser size and it should be fixed.

sidharthv96
sidharthv96 requested changes Jun 23, 2022
View reviewed changes
cypress/integration/rendering/conf-and-directives.spec.js
cy.get('svg').each(($el) => {
cy.wrap($el).toMatchImageSnapshot(name);
});
// cy.matchImageSnapshot('conf-and-directives.spec-when-rendering-several-diagrams-diagram-1');
Copy link
Member

Choose a reason for hiding this comment

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

Should clean up all these comments.

cypress/plugins/index.js
Comment on lines 21 to +23
module.exports = (on, config) => {
addMatchImageSnapshotPlugin(on, config);
// addMatchImageSnapshotPlugin(on, config);
};
Copy link
Member

Choose a reason for hiding this comment

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

Remove redundant module.exports

@knsv knsv deleted the e2e branch August 23, 2022 17:41
@knsv knsv restored the e2e branch August 23, 2022 17:41
@knsv knsv deleted the e2e branch August 23, 2022 17:41
@sidharthv96 sidharthv96 mentioned this pull request Sep 16, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sidharthv96 sidharthv96 sidharthv96 requested changes

@knsv knsv Awaiting requested review from knsv

Assignees
No one assigned
Labels
✔️ has tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Yash-Singh1 @knsv @sidharthv96 @ashishjain0512