feat: screenshot testing #10

patrickrodee · 2018-03-16T18:11:48Z

Add support for screenshot testing through Puppeteer. Tests run on Travis CI in a headless Chrome browser, parallel with the unit tests.

test: add Travis CI

moog16

Just left a comment. If you don't wanna change it LGTM

moog16 · 2018-03-16T18:30:13Z

scripts/upload-screenshots.js

+
+const screenshots = glob.sync(`${DIR}/**/*.png`);
+
+screenshots.forEach((fname) => {


I remember you changed this from fname --> screenshot

Yeah... changed it back.

moog16 · 2018-03-16T20:36:49Z

test/screenshot/temporary-package/temporary-package.scss

@@ -3,4 +3,6 @@

 body {
  color: blue;
+  -moz-osx-font-smoothing: grayscale;


never seen moz-osx 👍

Stole this from here

lynnmercier

YAY!

Write a README in test/screenshot explaining this workflow for both local/travisCI and new-feature/checking-for-regressions

lynnmercier · 2018-03-16T23:13:36Z

.travis.yml

+
+dist: trusty
+sudo: required
+


setup a matrix in travis.yml with two different jobs, see the MDC Web for an example.

lynnmercier · 2018-03-16T23:14:29Z

package.json

-    "test:ci": "karma start karma.ci.js --single-run",
-    "test:image-diff": "./node_modules/.bin/mocha --compilers js:babel-core/register --ui tdd --timeout 15000 test/screenshot/diff-suite.js"
+    "test:unit-ci": "karma start karma.ci.js --single-run",
+    "test:ci": "./scripts/run-ci-tests.sh",


I dont htink you'll need test:ci if you split up the travis.yml into two jobs

lynnmercier · 2018-03-16T23:15:09Z

scripts/run-ci-tests.sh

@@ -0,0 +1,36 @@
+#!/bin/bash


You don't need this file once you split up travis.yml into two jobs

lynnmercier · 2018-03-16T23:15:54Z

scripts/upload-screenshots.js

+const BUCKET_NAME = process.env.BUCKET_NAME;
+const COMMIT_HASH = process.env.COMMIT_HASH;
+const DIR = './test/screenshot';
+


lowercase your variables...no need to intense caps

lynnmercier · 2018-03-16T23:17:26Z

scripts/upload-screenshots.js

+ * limitations under the License.
+ */
+
+const PATH_TO_KEY = process.env.PATH_TO_KEY;


I think you don't need PATH_TO_KEY. I think the key is always at root.

lynnmercier · 2018-03-16T23:19:18Z

scripts/upload-screenshots.js

+const storage = new Storage({
+  keyFilename: PATH_TO_KEY,
+});
+


you have extra empty lines all over this file

lynnmercier · 2018-03-16T23:21:25Z

scripts/upload-screenshots.js

+
+screenshots.forEach((fname) => {
+  const fileName = path.resolve(fname);
+  const bucketFileName = fname.replace(DIR, COMMIT_HASH);


lets use PR instead of commit hash

Im worried that if we upload per commit to GCP, then we will end up storing a lot of files. At least with PR number you override the screenshots as you make changes within a PR.

And look into setting up a cron job in GCP to delete all the files in the screenshot-image-captures bucket.

lynnmercier · 2018-03-16T23:22:07Z

scripts/upload-screenshots.sh

@@ -0,0 +1,40 @@
+#!/bin/bash


This file should disappear

lynnmercier · 2018-03-16T23:25:22Z

test/screenshot/screenshot.js

+        readFilePromise(imagePath),
+      ]);
+
+      const options = {


don't redefine this variable everytime diff() is called. Make this a variable that screenshot.js imports

lynnmercier · 2018-03-16T23:26:25Z

test/screenshot/screenshot.js

+    const browser = await puppeteer.launch();
+    const page = await browser.newPage();
+    await page.goto(url);
+    if (path) {


You seem to always pass path into createScreenshotTask so you don't need this if check

lynnmercier · 2018-03-19T16:28:00Z

.travis.yml

+matrix:
+  include:
+    - node_js: 8
+      env: TEST_SUITE=unit


I dont think you need these environment variables anymore

I don't, but they help to provide names for the builds in Travis. Otherwise, you'd have to click into a build and parse the logs to figure out which test it is.

lynnmercier · 2018-03-19T16:31:05Z

scripts/README.md

@@ -0,0 +1,5 @@
+# Uploading screenshots


This readme should cover

running capture locally

running screenshot tests locally

how screenshot tests work in CI (aka where it uploads the diffs to)

And actually this should be under test/screenshot

lynnmercier · 2018-03-19T17:36:31Z

.travis.yml

+    - node_js: 8
+      env: TEST_SUITE=screenshots
+      install: npm i
+      script: npm run test:image-diff-ci


I suggest you run ./test/screenshot/start.sh && sleep 10s here before running npm run test:image-diff.

Then you don't even need the test:image-diff-ci NPM command

lynnmercier · 2018-03-19T17:37:18Z

.travis.yml

+sudo: required
+
+matrix:
+  include:


Add another job for just linting, it can run npm run lint

lynnmercier · 2018-03-19T17:39:21Z

.travis.yml

+      install: npm i
+      script: npm run test:image-diff-ci
+      after_script:
+        - echo $SERVICE_ACCOUNT_KEY > ./key.json


Can you not just reference SERVICE_ACCOUNT_KEY directly? Why do you have to write it to key.json?

From digging into the docs, it looks like there's support for passing the key directly as a credentials attribute instead of a keyFilename attribute.

I'll update to use that.

lynnmercier · 2018-03-19T17:39:45Z

scripts/README.md

@@ -0,0 +1,5 @@
+# Uploading screenshots


And actually this should be under test/screenshot

lynnmercier · 2018-03-19T17:39:53Z

scripts/upload-screenshots.js

@@ -0,0 +1,50 @@
+/**


Move this to test/screenshot directory

lynnmercier

YAY

patrickrodee added 30 commits March 15, 2018 13:29

test: added travis!

1aaf166

test: updated package-lock.json

2fbb557

test: updated travis config

8a437cc

Merge pull request #9 from material-components/experimental/travis

4ec83ee

test: add Travis CI

WIP adding screenshot testing

ece41ce

WIP added travis script here we go!

6b382bc

WIP added screenshot uploading

fcbabfd

WIP changed test name because i forgot...

6628e05

WIP fixed screenshot naming...

a0cc88f

WIP fixed file naming in screenshot upload

176741b

WIP added explicit fonts to screenshot html files

a17fabc

WIP increased timeout; went back to commit for url

70f778e

WIP testing sandbox and explicit Roboto

e5c87ce

WIP made index.html white text on blue background

0f5f663

WIP adding platform-specific images [skip-ci]

cc17804

WIP added linux images

e34535a

WIP new screenshot shiz [skip ci]

4827234

WIP added new images; testing on OSX

443c3d8

WIP removed platform-specific images

8c76c7f

WIP added Arial to fallback font styles

57a03e4

WIP updated travis to run Trusty with sudo

8ebd2ea

WIP set body color to hardcoded rgb value

340d30c

WIP testing antialised fonts

7c8306e

WIP testing with mdc font smoothing

4e369c6

WIP attempting to ignore antialiasing

93f355c

WIP testing osx 10.13

d37d1a2

WIP testing no anti-aliasing on all linux

2647f69

WIP moved image upload to after_script

805470d

WIP lowered diff threshold; cleaned up travis.yml

7eb72f1

WIP setup screenshot testing to run only on master

2275fbb

patrickrodee requested review from lynnmercier and moog16 March 16, 2018 18:12

moog16 approved these changes Mar 16, 2018

View reviewed changes

patrickrodee force-pushed the master branch from 4ec83ee to 0c6a581 Compare March 16, 2018 21:02

lynnmercier suggested changes Mar 16, 2018

View reviewed changes

patrickrodee added 6 commits March 16, 2018 16:38

WIP reorganized screenshot testing

bc5c8ea

WIP testing more screenshot PR number work

968097f

WIP cleaned up travis.yml

6258338

WIP added copyright header

7269996

WIP debugging what's happening with Travis

4da5232

WIP removed debug line from travis; added readme to screenshot uploader

4fa818a

lynnmercier suggested changes Mar 19, 2018

View reviewed changes

patrickrodee added 2 commits March 19, 2018 13:11

WIP updated readme; moved upload scripts; added linting to ci

d421f42

WIP updated readme; renamed "test.png" to "snapshot.png"

f8da262

lynnmercier approved these changes Mar 19, 2018

View reviewed changes

Merge branch 'master' into experimental/screenshot-testing

ea1085a

patrickrodee merged commit 5fd6d86 into master Mar 19, 2018

moog16 deleted the experimental/screenshot-testing branch July 17, 2018 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: screenshot testing #10

feat: screenshot testing #10

patrickrodee commented Mar 16, 2018

moog16 left a comment

moog16 Mar 16, 2018

patrickrodee Mar 16, 2018

moog16 Mar 16, 2018

patrickrodee Mar 16, 2018

lynnmercier left a comment

lynnmercier Mar 16, 2018

lynnmercier Mar 16, 2018

patrickrodee Mar 16, 2018

lynnmercier Mar 16, 2018

lynnmercier Mar 16, 2018

lynnmercier Mar 16, 2018

lynnmercier Mar 16, 2018

lynnmercier Mar 16, 2018

lynnmercier Mar 16, 2018

lynnmercier Mar 16, 2018

lynnmercier Mar 16, 2018

lynnmercier Mar 19, 2018

patrickrodee Mar 19, 2018

lynnmercier Mar 19, 2018

lynnmercier Mar 19, 2018

lynnmercier Mar 19, 2018

patrickrodee Mar 19, 2018

lynnmercier Mar 19, 2018

patrickrodee Mar 19, 2018

lynnmercier Mar 19, 2018

patrickrodee Mar 19, 2018

lynnmercier Mar 19, 2018

lynnmercier Mar 19, 2018

patrickrodee Mar 19, 2018

lynnmercier left a comment


		const screenshots = glob.sync(`${DIR}/*/.png`);

		screenshots.forEach((fname) => {


		dist: trusty
		sudo: required

feat: screenshot testing #10

feat: screenshot testing #10

Conversation

patrickrodee commented Mar 16, 2018

moog16 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lynnmercier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lynnmercier left a comment

Choose a reason for hiding this comment