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

Add tests for Image Heatmap question creation and response #83

Merged
merged 16 commits into from
Nov 3, 2021

Conversation

jxjj
Copy link
Contributor

@jxjj jxjj commented Nov 1, 2021

This adds tests for image heatmap questions via cypress image snapshots. 🖼️ ✅ 👏

Notable... err... notes:

  • New dev dependency: cypress-image-snapshot, which is based on americanexpress/jest-image-snapshot.
  • Change .env.example so that, by default, DEBUG_BAR=false. Debug bar will cause all sorts of false positives with image diffing, and cur CI workflow .env file is a copy of .env.example. So, this makes sure DEBUG_BAR won't be in any CI screenshots.
  • cypress/<testname>/__diff_output__ will contain any generated diffs from failing snapshot tests 😎. Add this to .gitignore to prevent accidental committing.
  • cypress/plugins/index.js is renamed to index.ts. We're using type: module in this project now, but cypress doesn't support ESM or.cjs file extensions yet. A workaround is naming it with a .ts extension so that cypress parses the file as if it were typescript.
  • Testing image snapshots was a little flakey due to timing issues. Using cy.intercept to wait for the PUT request helped. I also changed the diff threshold to 0.5% to reduce false positives.

Thanks for the review!

@jxjj jxjj requested a review from cmcfadden November 1, 2021 21:03
Copy link
Member

@cmcfadden cmcfadden left a comment

Choose a reason for hiding this comment

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

this is super cool. I'll be interested to live with it for a bit and get a sense of whether it ends up flapping much

@jxjj
Copy link
Contributor Author

jxjj commented Nov 1, 2021

Let the flapping begin...

I'll have to make a few changes tomorrow before merging. The local images and the ones generated in CI are different sizes. Probably need to generate the shots using the cypress docker image like CI uses.

@jxjj
Copy link
Contributor Author

jxjj commented Nov 3, 2021

TLDR

So, this now passes consistently in headless mode (cypress run) both locally and in CI.

When cypress is in interactive mode (cypress open), the test will likely fail due to environment differences: OS, browser, default fonts, screen size, dpi, etc all impact the screenshot.

More details

Screenshot size was the biggest problem for me when not in headless. Even within the same environment, I found that running tests on my iPad screen gave different results from my 13" macbook pro (even when specifically told to use a window specific browser size like of 1920x1080. Presumably due to pixels per inch differences between the screens (264 in iPad Pro vs 227 in Macbook Pro)?

This seems to be a common issue with Cypress and visual regression testing. More Discussion:

Cypress' line is that one should ensure consistent environment when testing for visual regressions... which makes sense I guess.

I wonder if 3rd party services like Percy or Applitools solve this problem?

Interestingly, it looks like jest-image-snapshot – which the cypress package is base one – is working on a structural similarity (SSIM) approach rather than a direct pixel-by-pixel comparison. Also, they have implemented the ability to ignore size mismatches. However, neither option worked for me in the cypress snapshot package.

I'll merge this test for now. Like you mentioned, we can explore an alternate approach if it flakes.

@jxjj jxjj merged commit 52f0100 into develop Nov 3, 2021
@jxjj
Copy link
Contributor Author

jxjj commented Nov 3, 2021

Oh! I should also mention I explored just moving to all local cypress tests to their docker image. I kept running into roadblocks trying to run this on the M1. Maybe in the future?

@cmcfadden cmcfadden deleted the feature/test-image-heatmap branch November 24, 2021 14:45
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.

2 participants