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 comparison in CI #3894

Closed
wants to merge 29 commits into from
Closed

E2E comparison in CI #3894

wants to merge 29 commits into from

Conversation

sidharthv96
Copy link
Member

📑 Summary

Run e2e image diff tests in CI.

TODO: Test ssim diff algorithm, if tests are flaky.

📏 Design Decisions

Screenshots are checked into the repo.
If there are issues, we can use

📋 Tasks

Make sure you

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

sidharthv96 and others added 29 commits December 7, 2022 13:26
* develop: (44 commits)
  Update docs
  Add `workflow_dispatch` to lint.yml
  chore: update docs folder
  lint
  fix typescript error
  fix(docs): build the docs
  fix(docs): remove duplicate section
  chore(deps): update all non-major dependencies
  Update docs/misc/integrations.md
  Add links to github atom add-ons
  remove links from atom.io; add note Atom has been archived
  set svg role to 'graphics-document document'
  common function for a11y; add to renderAsync; + renderAsync spec
  feat: add links to theme listing
  fix cspell
  fix cspell
  fix lint
  refactor theming doc
  remove typeof
  use camelCase
  ...
@sidharthv96
Copy link
Member Author

@aloisklink, I need some help here...

There are around 6 failing diagrams now, which are available in the last failed test here
Few failures seem to be lack of Chinese(?) font in the CI system. I tried adding some CSS fonts, but that didn't work.
Those def need to be fixed before we merge. Would installing the font in system help? I'm not really sure.

For some others, it is subtle rendering differences between CI (ubuntu) & my machine (macOS).
What approaches can we have to make sure the rendering is consistent? Maybe use a docker container to render snapshots in local?

Furthermore, what should the ideal dev flow look like?

  1. Developer commits code and pushes
  2. CI runs snapshot test and updates changed snapshots, pushes
  3. PR is tagged as snapshots-changed, so it requires review from @knsv
  4. We review and merge the PR

In this case, we can skip the local rendering and let the CI handle it.

@aloisklink
Copy link
Member

aloisklink commented Dec 22, 2022

Would installing the font in system help? I'm not really sure.

Yep! It should do. If you give me a link to the Mermaid diagram that is failing to render, I can try investigating which font it uses locally, so we can add it to CI.

There's more discussion in #3491 that might help :) (we should probably added Closes https://github.com/mermaid-js/mermaid/issues/3491 to your PR description too).

For some others, it is subtle rendering differences between CI (ubuntu) & my machine (macOS).

You can install sudo apt-get install ttf-mscorefonts-installer to install the "trebuchet ms" font, which is the default font for Mermaid.

Even on MacOS, these will normally be installed, since they come with Microsoft Office and other Microsoft programs, that you'll normally install on a Mac.

https://github.com/mermaid-js/mermaid/blob/03c5bc112947eebb428e1d540a665392b5eb800c/packages/mermaid/src/defaultConfig.ts#LL66C17-L66C29

However, installing ttf-mscorefonts-installer requires accepting a Microsoft license agreement! You can run echo ttf-mscorefonts-installer msttcorefonts/accepted-mscorefonts-eula select true | sudo debconf-set-selections to accept this automatically, but we need to make sure that we're complying with the license before using it.

What approaches can we have to make sure the rendering is consistent? Maybe use a docker container to render snapshots in local?

Docker won't help if you're running on an ARM based system, since then all of your Docker images are ARM-based too, and so things like CSS shadows might be slightly different compared to x86_64 machines.

(floating point numbers are always fun!! 0.1 * 3 === 0.3 returns false in JavaScript.)

I believe due to ClearType/FreeType, you might also get different results depending on what your sub-pixel arrangement your computer's monitor has 😱

The only way to make rendering consistent is to only render in CI on the same OS.

CI runs snapshot test and updates changed snapshots, pushes

CI pushing to a PR gets messy, especially if people are making PRs from forks.

Plus, if possible, we shouldn't store snapshots in the main Mermaid repo, as it will blow up the size of the Git repo (if a snapshot changes 100 times, then each time somebody git clone the repo, we'd have to download 100 images, since the Git repo stores the full history of the images).

How about this instead:

  • CI runs first on the parent to calculate the before snapshots. We can use https://github.com/actions/cache to automatically cache these snapshots, so normally, this shouldn't be slow, since they'll be pre-calculated.
    • If making a PR, CI runs snapshot tests on github.head_ref first (aka develop branch commit)
    • If making a commit, push, CI runs snapshot tests on the parent commit of github.sha (for merge commits, we should pick whichever one follows the develop branch (maybe first parent?))
  • CI then runs tests on new commit (aka github.sha).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Would it make sense to checkin Cypress e2e snapshots?
2 participants