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

test(happo): add Happo for screenshot testing #1030

Merged
merged 17 commits into from
Jan 26, 2021
Merged

Conversation

williaster
Copy link
Collaborator

@williaster williaster commented Jan 22, 2021

🏠 Internal

This PR adds happo.io to visx so that we have more robust visual testing in our CI 🍾 🥳 .

It specifically

  • adds a happo job to our github ci workflow
    • split ci from one job into build/test/lint/happo jobs – would love 👀 on this because I'm new to github actions
      • EDIT: I undid this because the output of a build job can't be used by lint/test/happo
    • enabled the happo app so it can post on PRs
    • added appropriate secrets to the repo
    • setup account details with @lencioni
  • configured happo to run on all of our @visx/demo gallery tiles
    • added happo.io dep + configuration
      • specifically I am using the next.js-oriented configuration which mostly just leverages the next.js webpack config
    • fixed some copy/paste issues from the sandbox examples to ensure a unique test names (taken from sandbox package.json name field)

Locally I was able to test it on all examples (here's a sample report image), will see how CI goes 🤞

@hshoff @kristw @lencioni

@@ -66,6 +68,7 @@
"@visx/xychart": "1.4.0",
"@visx/zoom": "1.3.0",
"@zeit/next-css": "^1.0.1",
"babel-loader": "8.2.2",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

needed by happo.io

Copy link

@trotzig trotzig left a comment

Choose a reason for hiding this comment

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

Nice!

I can't see the CI build results, so I can't help you there unfortunately.


// happo is unable to resolve some imports if the tmpdir isn't located inside
// the project structure. The default is an OS provided folder, `os.tmpdir()`.
tmpdir: path.join(__dirname, happoTmpDir),
Copy link

Choose a reason for hiding this comment

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

Good catch, I've seen this happen myself. I'll make a note to change the default tmp folder to something like ./.happo-tmp.

packages/visx-demo/happo/gallery.tsx Outdated Show resolved Hide resolved
},
},
},
// needs timeout for animated axes
Copy link
Contributor

Choose a reason for hiding this comment

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

One option that you might want to consider is to not use an animation if prefers-reduced-motion is set to reduce, and then add prefersReducedMotion: true, to your RemoteBrowserTarget:

'chrome-desktop': new RemoteBrowserTarget('chrome', {
  viewport: '800x552',
  prefersReducedMotion: true,
}),

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is a cool idea. it'll be a bit of work to re-write those examples because the animations aren't css-based. looks like you can do the media query in JS to detect it, so I can swap out animated components for their static analogs. I may do this in a follow up PR depending on how big the change is.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
};
};

const MAX_TIMEOUT_MS = 400;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 400? Is there a variable somewhere else that this could import and be based on?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ideally this is in sync with .happo.js, will update to import from there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

couldn't import asyncTimeout directly from .happo.js because node's child_process gets pulled into the webpack config step which throws. put it in a separate file.

@coveralls
Copy link

coveralls commented Jan 22, 2021

Pull Request Test Coverage Report for Build 513163337

  • 6 of 6 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 61.423%

Totals Coverage Status
Change from base Build 499518348: 0.09%
Covered Lines: 1740
Relevant Lines: 2702

💛 - Coveralls

env:
HAPPO_API_KEY: ${{ secrets.HAPPO_API_KEY }}
HAPPO_API_SECRET: ${{ secrets.HAPPO_API_SECRET }}
HAPPO_COMMAND: '../../node_modules/happo.io/build/cli.js'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

noting that the happo-ci binary expects this path to be in the same directory, but because this is run in ./packages/visx-demo/ it needs to point to the monorepo root node_modules where yarn installs it.

@williaster
Copy link
Collaborator Author

🎉 report looks good. I'm going to create one more PR to add a stable Math.random to use in examples so that we don't get spurious diffs (this is used across ~5 demos right now). That can merge and then we can merge this one.

Copy link
Collaborator

@kristw kristw left a comment

Choose a reason for hiding this comment

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

super cool

packages/visx-demo/happo/gallery.tsx Outdated Show resolved Hide resolved
williaster and others added 2 commits January 26, 2021 11:53
* deps(mock-data): d3-random@^2.2.2 for seeded random

* new(mock-data): add getSeededRandom, support seeds in all generators

* test(mock-data): add tests for seeded random across generators

* new(mock-data): export getRandomNormal

* internal(demo): use stable randomness for all demos
@williaster williaster merged commit 1dafaa3 into master Jan 26, 2021
@williaster williaster deleted the chris--happo-ii branch January 26, 2021 20:30
@williaster williaster added this to the 1.5.0 milestone Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants