Update Cypress to v10 and fix E2E errors #3459

aloisklink · 2022-09-14T06:46:52Z

📑 Summary

A bunch of E2E tests have been failing under Cypress 9.7.0 due to SIGSEGV (Segmentation fault) errors. I'm guessing this is because Cypress is slowly losing compatibility with current web browsers.

Upgrading to Cypress v10.8.0 has fixed those SIGSEGV errors.

This upgrade has been attempted before, but @knsv reverted it to fix Applitools integration (see dee9cfe).

I've tested it with own Applitools account, and it seems to work!

Additionally, I've got E2E and Applitools working in GitHub Actions (although it does have some issues, so it might not be worth keeping, see below).

📏 Design Decisions

I've also fixed every other currently failing e2e test, so that CI passes.

test(e2e): fix failing xss9 test
This just needed a small change to get Cypress to continue running, even if mermaid threw an exception.
test(e2e): widen flowchart width to within 10%
Widen flowchart width acceptable values to within 10% of 300px. On my local environment, I'm getting 283px, which is 5.5% of 300px.
text(e2e): give git tests consistent commit id
(to prevent snapshots changing in-between runs)
test(e2e): fix most arrowMarkerAbsolute tests
The .edgePath class they were searching for no longer exists.

One test was broken due to a regression in flowchart (the arrowMarkerAbsolute config setting no longer does anything). I've done a quick and ugly fix for flowchart-v2, but it's still broken in flowchart, see fix(flowchart-v2): fix arrowMarkerAbsolute=true.

Applitools Configuration

I've added Applitools to the e2e GitHub Actions CI and it works on my aloisklink/mermaid repo.

Issues:

It relies on the APPLITOOLS_API_KEY GitHub Secret being set.
This means that PRs from forks (like in this PR) will not use applitools, since GitHub prevents PRs from forks from accessing any GitHub Secret for security reasons.
Running this on every push might cause a bunch of unnecessary runs, which might be a bit slow/use up a lot of applitools credits. It might be worth changing this so it only runs on pushes and PRs to the develop branch.

📋 Tasks

Make sure you

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

Fixes xx9 test to continue running even if JavaScript throws an exception.

Widen flowchart width acceptable values to within 10% of 300px. On my local environment, I'm getting 283px, which is 5.5% of 300px.

gitGraph render tests are currently using random commit ids, which causes visual regression tests to fail. I couldn't find a way to make the random commit IDs consistent (JavaScript/Cypress doesn't seem to have a random.seed() function), so I've just hardcoded all the commit ids.

The arrows between flowcharts do not have the class edgePath. Instead, I'm loading all `<path>`s within the `<g class='edgePaths'>`

The if-check for arrowMarkerAbsolute in the flowchart-v2 code is in dagre-wrapper. Unfortunately, I can't seem to find a way to find the local conf (e.g. the one set via `flowRenderer.setConf()`, so instead I'm using global mermaid config from `src/config`. Flowchart v1 arrowMarkerAbsolute=true is still broken, but I'm not really sure how to fix that.

Cypress v10 had some minor breaking changes, (e.g. the cypress/plugins folder dissapeared) so I had to change some things. It looks like the Cypress was previously reverted in [dee9cfe][1], due to applitools not working properly, but I think my changes fix this, so it should be okay. [1]: mermaid-js@dee9cfe

Uses prettier to fix e2e style issues.

I've removed the coverage upload bit, since although the [@cypress/code-coverage][1] plugin is pretty easy to setup, it requires babel, which we're about to remove and replace with ESBuild.

knsv · 2022-09-16T05:58:53Z

This is great!

Nice to finally to be able to use cypress 10 and not lagging behind. This is a really important part of our release process so it is worth getting right. We have had issues in the past where the quota has been blown away by PRs causing issues with the release flow!

It is good to have it running using actions to keep it consistent and not depending on you local computer.

How and when to trigger the tests

Ideally we want this run on internal and external PRs
We don't want to trigger on every dependabot PR, rather grouping it to be run after a batch of dependabots
Is there a way to trigger this manually for select PRs?

@aloisklink Maybe we can start by having it run manually and add automatic steps in iterations. A no-brainer is to have it run when we branch to a release branch.

knsv · 2022-09-16T06:01:00Z

.github/workflows/e2e.yml

@@ -0,0 +1,61 @@
+name: E2E
+
+on: [push, pull_request]


@aloisklink Se my comment regarding when to trigger

I've deleted the commit adding Applitools to CI, and will create a new PR that adds a e2e-applitool.yml CI action.

The e2e.yml test now doesn't use Applitools, so it's fine to have it run on every push/PR, since GitHub Actions is currently completely free for open-source projects.

knsv · 2022-09-16T06:09:05Z

.github/workflows/e2e.yml

+        env:
+          # e.g. mermaid-js/mermaid/my-branch
+          APPLITOOLS_BRANCH: ${{ github.repository }}/${{ github.ref_name }}
+          APPLITOOLS_PARENT_BRANCH: develop


I read this is at it creates a branch name from the github branch in question and compares this will the develop baseline?
This is good for all develop related comparisons but in the release flow we want to compare with last release baseline. This gives an extra layer of security as it is possible to falsly approve a snapshot in develop.

I've removed the Applitools CI action from this PR, and will fix this in another PR once I figure out the best way to manually trigger workflows.

cypress/integration/rendering/gitGraph.spec.js

knsv · 2022-09-16T06:40:17Z

src/diagram-api/diagram-orchestration.ts

        if (!cnf.flowchart) {
          cnf.flowchart = {};
        }
+        // TODO, broken as of 2022-09-14 (13809b50251845475e6dca65cc395761be38fbd2)


👍 Will fix in another PR

aloisklink · 2022-09-16T08:23:53Z

Hi @knsv, it looks like getting Applitools running on GitHub Actions will be a bit difficult.

How about I delete commit dbbe95c in this PR?

Instead, this PR's e2e.yml testing will just do generic Cypress tests on each PR/push (even dependabot PRs) without applitools. It won't have visual regression testing, but at least it will test if all the rendering tests pass!

Then I'll make another PR that adds e2e-applitools.yml, which will be a new action that we can run manually, so that we avoid using the applitools quota.

Ideally we want this run on internal and external PRs

This might be difficult for external PRs. The workflow_dispatch action that lets you do manual GitHub Actions only works for internal branches. I'll try to get the action setup for internal branches only first, then do some research on how this might be done.

@aloisklink Maybe we can start by having it run manually and add automatic steps in iterations. A no-brainer is to have it run when we branch to a release branch.

👍 Sounds good to me :)

sidharthv96 · 2022-09-16T11:59:18Z

Do you think we can keep the screenshots in the repo and run local verification instead of relying on applitools for PRs?
We could still use applitools before release, but a test running for every PR would help both authors and maintainers.

chore: fix e2e #2961 might be related.

aloisklink · 2022-09-16T13:12:17Z

Do you think we can keep the screenshots in the repo and run local verification instead of relying on applitools for PRs?

Two issues I see with this:

The cypress/snapshots folder is about 8 MB on my PC. If we start updating those snapshots, the size of our git history will get larger and larger over time.
We could use something like Git LFS, but the problem is:
- you need to install Git LFS separately
- GitHub charges for Git LFS bandwidth, so it get's expensive very quickly (I don't think it's free for open-source)
Local verification will throw a lot of tiny rendering differences.
- Mermaid's default font is "trebuchet ms", which is a Microsoft owned font, so it might not be installed on many PCs.
  
  mermaid/src/defaultConfig.ts
  
  Line 50 in 37eb045
  
  fontFamily: '"trebuchet ms", verdana, arial, sans-serif;',
- Apparently there are slight differences in how Apple Silicon (ARM) and Intel calculate shadows (Advice for handling visual discrepancies between Intel and Apple Silicon images americanexpress/jest-image-snapshot#302)

aloisklink · 2022-09-16T13:22:13Z

I've removed commit dbbe95c which added Applitools to GitHub Actions CI. This PR still adds e2e rendering tests, even if it doesn't have visual regression tests.

I'll make another PR that adds Applitools E2E tests on CI, since that will get a bit of working out to setup.

sidharthv96 · 2022-09-16T13:34:22Z

Mermaid's default font is "trebuchet ms", which is a Microsoft owned font, so it might not be installed on many PCs.

@knsv Not related to cypress, but shouldn't we change to a properly "open" font?

Another option is to run tests twice.

Checkout develop
Generate baseline
Checkout branch
Run tests

It'll take some CI time, but will make the lives of the maintainers easier.

aloisklink · 2022-09-16T14:01:12Z

Not related to cypress, but shouldn't we change to a properly "open" font?

It's not too big of a deal, since browsers will just fallback to other similar fonts.

If we do switch, we may want to use a webfont (I like Atkinson Hyperlegible, since it's designed for people with low vision), since then everybody's browser will download the same font.

But then again, webfonts have issues with download speeds, offline browsers, and privacy concerns (if we use a CDN).

Another option is to run tests twice.

Checkout develop

Generate baseline

Checkout branch

Run tests

I can't believe I didn't think of that! That's work pretty well. It will probably get a bit more complicated, since we'd also need a way to view the image differences, but I think that should just be uploading a __diff_upload__ file (somebody started cypress-image-snapshot-review-action last month, which is exactly what we need, but it's not yet finished).

sidharthv96 · 2022-09-16T14:04:11Z

We can store that as artifacts with a lifetime of 3-5days.
https://docs.github.com/en/actions/using-workflows/storing-workflow-data-as-artifacts

knsv · 2022-09-16T15:33:21Z

You guys have been active in this discussion 👍

About the font, there is nothing special with Trebuchet other then that it is a web-safe font. Lets keep until there is an issue with it. In the cypress tests there should be a wider font. We had issues for a while in the beginning, with cut text in the boxes resulting from the boxes being sized with a more narrow font than the one used in the final presentation. It is good to have that as a part of the automatic tests as worst case scenarios.

I understand that by merging this we have the appli tools run manually for now. ✅

The problem with having baseline images compared between different environments are very real. Slight changes in antialiasing between for instance linux and mac causes issues. So the method of running the tests twice would work better than the other options. So I am in favour of that.

We should document and strongly promote to use a similar process to the PR authors.
When creating a PR:

start by generating a local baseline form develop
add the changes
run the e2e tests and have them ok before pushing the PR
This will ease some frustrations that will come from receiving a fail after 20+ minutes.

knsv · 2022-09-16T15:34:02Z

So merge now and add the other stuff in next PR?

aloisklink · 2022-09-16T16:32:01Z

So merge now and add the other stuff in next PR?

Sounds good to me 👍 Since we're not adding the Applitools stuff in this PR, I think it's relatively low risk to merge now.

In the worst case scenario, we can always do a git revert 625ec813b9459740320730b38cb94b5e1ccdeafe to revert the Cypress v9 to v10 upgrade.

aloisklink added 8 commits September 14, 2022 04:15

test(e2e): fix failing xss9 test

f05c790

Fixes xx9 test to continue running even if JavaScript throws an exception.

test(e2e): widen flowchart width to within 10%

05b8a6e

Widen flowchart width acceptable values to within 10% of 300px. On my local environment, I'm getting 283px, which is 5.5% of 300px.

test(e2e): fix most arrowMarkerAbsolute tests

6e7037b

The arrows between flowcharts do not have the class edgePath. Instead, I'm loading all `<path>`s within the `<g class='edgePaths'>`

style: fix .github/workflow/e2e styling

c5033b9

Uses prettier to fix e2e style issues.

ci(e2e): re-enable e2e tests

20d22a6

I've removed the coverage upload bit, since although the [@cypress/code-coverage][1] plugin is pretty easy to setup, it requires babel, which we're about to remove and replace with ESBuild.

aloisklink added the Area: DevOps label Sep 14, 2022

aloisklink requested a review from knsv September 14, 2022 06:46

github-actions bot added the chore label Sep 14, 2022

knsv reviewed Sep 16, 2022

View reviewed changes

aloisklink force-pushed the chore/update-cypress branch from dbbe95c to 20d22a6 Compare September 16, 2022 13:22

aloisklink merged commit e8eb2ab into mermaid-js:develop Sep 16, 2022

aloisklink deleted the chore/update-cypress branch September 16, 2022 16:32

This was referenced Sep 18, 2022

ci(e2e-applitols): add applitools CI action #3483

Merged

Would it make sense to checkin Cypress e2e snapshots? #3491

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Cypress to v10 and fix E2E errors #3459

Update Cypress to v10 and fix E2E errors #3459

aloisklink commented Sep 14, 2022

knsv commented Sep 16, 2022

knsv Sep 16, 2022

aloisklink Sep 16, 2022

knsv Sep 16, 2022

aloisklink Sep 16, 2022

knsv Sep 16, 2022

aloisklink commented Sep 16, 2022

sidharthv96 commented Sep 16, 2022

aloisklink commented Sep 16, 2022

aloisklink commented Sep 16, 2022

sidharthv96 commented Sep 16, 2022

aloisklink commented Sep 16, 2022

sidharthv96 commented Sep 16, 2022

knsv commented Sep 16, 2022

knsv commented Sep 16, 2022

aloisklink commented Sep 16, 2022

Update Cypress to v10 and fix E2E errors #3459

Update Cypress to v10 and fix E2E errors #3459

Conversation

aloisklink commented Sep 14, 2022

📑 Summary

📏 Design Decisions

Applitools Configuration

📋 Tasks

knsv commented Sep 16, 2022

knsv Sep 16, 2022

Choose a reason for hiding this comment

aloisklink Sep 16, 2022

Choose a reason for hiding this comment

knsv Sep 16, 2022

Choose a reason for hiding this comment

aloisklink Sep 16, 2022

Choose a reason for hiding this comment

knsv Sep 16, 2022

Choose a reason for hiding this comment

aloisklink commented Sep 16, 2022

sidharthv96 commented Sep 16, 2022

aloisklink commented Sep 16, 2022

aloisklink commented Sep 16, 2022

sidharthv96 commented Sep 16, 2022

aloisklink commented Sep 16, 2022

sidharthv96 commented Sep 16, 2022

knsv commented Sep 16, 2022

knsv commented Sep 16, 2022

aloisklink commented Sep 16, 2022