feat: remote hosted screenshot testing #12

patrickrodee · 2018-03-22T20:54:09Z

Remove repo screenshots in favor of Google Cloud Storage-hosted images. Add golden.json file to keep track of the golden image per tested URL. Increase line length to 120. Remove image upload step from Travis CI.

acdvorak

Looks really good overall!

acdvorak · 2018-03-22T21:01:52Z

test/screenshot/golden.json

+{
+  "temporary-package/index.html": "57de91e42bd22a845074bae80f71eca3902ab7c6719f129960bc9018c79db95a",
+  "temporary-package/foo.html": "91f0795700eaba345092dd7507e046daa0d33e1136bad7992011ab454ddb1faf"
+}


Nit: Add trailing newline to end of file (and probably update your editor config to do this automatically)

acdvorak · 2018-03-22T21:03:17Z

package.json

@@ -5,7 +5,7 @@
  "scripts": {
    "start": "webpack-dev-server --config test/screenshot/webpack.config.js --content-base test/screenshot",
    "stop": "./test/screenshot/stop.sh",
-    "capture": "./node_modules/.bin/mocha --compilers js:babel-core/register --ui tdd test/screenshot/capture-suite.js",
+    "capture": "COMMIT_HASH=$(git rev-parse --short HEAD) BRANCH_NAME=$(git rev-parse --abbrev-ref HEAD) mocha --compilers js:babel-core/register --ui tdd --timeout 30000 test/screenshot/capture-suite.js",


If we want this to run on Windows, we'll need to use cross-env (see MDC Web's package.json for examples).

Also, keep in mind that there may be unstaged changes in the working directory that won't be reflected in COMMIT_HASH. This may end up biting us down the road if we forget that fact and assume that screenshots with a particular commit hash are from that exact commit with no other changes.

acdvorak · 2018-03-22T21:06:15Z

test/screenshot/screenshot.js


 import comparisonOptions from './screenshot-comparison-options';

 const readFilePromise = promisify(readFile);
 const writeFilePromise = promisify(writeFile);

+const SERVICE_ACCOUNT_KEY = process.env.SERVICE_ACCOUNT_KEY;


I would suggest prefixing all of our env vars with MDC_. This makes it easier to tell which part of the stack is using the var, and makes it easier to search/replace.

Also, we might want to rename this var to indicate that it's used by GCP.

E.g., MDC_GCP_SERVICE_ACCOUNT_KEY or MDC_GCLOUD_SERVICE_ACCOUNT_KEY.

acdvorak · 2018-03-22T21:12:35Z

test/screenshot/screenshot.js

+  credentials = JSON.parse(SERVICE_ACCOUNT_KEY);
+} catch (err) {
+  console.error('Service account key could not be parsed.');
+  process.exit(1);


This will break any future unit tests. Maybe just rethrow the error instead?

acdvorak · 2018-03-22T21:15:32Z

test/screenshot/screenshot.js

+   * @private
+   */
+  generateImageID_(imageBuffer) {
+    return createHash('sha256').update(imageBuffer).digest('hex');


Why not sha512?

Changing your hashing algorithm in the future is painful (just ask git or npm), so we should pick the "right" one from the start.

No reason, I just chose the first one that came to mind

acdvorak · 2018-03-22T21:17:56Z

test/screenshot/screenshot.js

+
+      case SNAPSHOT:
+      case DIFF:
+      return `${this.urlPath_}/${COMMIT_HASH}/${imageId}.${imageType}.png`;


Nit: Indent

acdvorak · 2018-03-22T21:20:14Z

test/screenshot/screenshot.js

+   * Saves the given image to Google Cloud Storage with optional metadata
+   * @param {string} imagePath The path to the image
+   * @param {!Buffer} imageBuffer The image buffer
+   * @param {Object} customMetadata Optional custom metadata


Nit: @param {!Object=}

acdvorak · 2018-03-22T21:21:06Z

test/screenshot/screenshot.js

-      const url = `http://localhost:8080/${this.urlPath_}`;
-      const imagePath = `./test/screenshot/${this.imagePath_}`;
-      await this.createScreenshotTask_(url, imagePath);
+      const golden = await this.createScreenshotTask_();


Rename to goldenTask or screenshotTask?

Could rename the method to takeScreenshot_ since it returns a Buffer, not a task.

acdvorak · 2018-03-22T21:22:26Z

test/screenshot/screenshot.js

+  }
+
+  /**
+   * Captures a screenshot of the test URL


Captures a screenshot of the test URL and marks it as the new golden image

acdvorak · 2018-03-22T21:24:37Z

test/screenshot/screenshot.js

+
+  /**
+   * Captures a screenshot of the test URL
+   */
  capture() {


The name capture is misleading. It would be clearer to name it something like updateGolden.

lynnmercier · 2018-03-22T23:19:48Z

test/screenshot/screenshot.js


 import comparisonOptions from './screenshot-comparison-options';

 const readFilePromise = promisify(readFile);
 const writeFilePromise = promisify(writeFile);

+const SERVICE_ACCOUNT_KEY = process.env.MDC_GCLOUD_SERVICE_ACCOUNT_KEY;


dont need all caps constants

Are you sure?

The Google style guide says that constants should be all uppercase.

lynnmercier · 2018-03-22T23:20:34Z

test/screenshot/screenshot.js

+  /**
+   * Returns the golden hash
+   * @return {string|undefined}
+   * @private


sort private methods to the bottom of the file

lynnmercier · 2018-03-22T23:22:24Z

test/screenshot/screenshot.js

+  }
+
+  /**
+   * Saves the golden hash


hehehe, golden hash

acdvorak · 2018-03-23T18:43:53Z

test/screenshot/screenshot.js

+  /**
+   * Returns the correct image path
+   * @param {string} imageHash The image hash
+   * @param {string} imageType The image type


Can we make an @enum for this? It can have string values so that your existing usages will continue to work.

acdvorak · 2018-03-23T20:38:22Z

test/screenshot/screenshot.js

+        this.saveImage_(diffPath, diff, metadata),
+      ]);
+
+      return assert.isBelow(Number(data.misMatchPercentage), 0.01);


Make a constant for .01.

acdvorak · 2018-03-23T20:42:59Z

test/screenshot/screenshot.js

+    // Create a new stream from the image buffer
+    let stream = new Readable();
+    stream.push(imageBuffer);
+    stream.push(null);


Maybe add a comment on this line explaining that null indicates EOF

acdvorak · 2018-03-23T20:43:38Z

test/screenshot/upload-screenshots.js

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


Don't forget to remove or update npm run upload:screenshots in package.json

acdvorak

Just a few minor comments, otherwise LGTM!

patrickrodee added 2 commits March 22, 2018 13:47

WIP add screenshot testing with GCP image hosting; removed local images

d001ef7

WIP removed upload step from travis

a508d1d

patrickrodee requested a review from acdvorak March 22, 2018 21:14

acdvorak suggested changes Mar 22, 2018

View reviewed changes

patrickrodee added 6 commits March 22, 2018 15:02

WIP updated from review

5648521

WIP passes lint yay

4182565

WIP set travis to run only on master

61c9583

WIP sha512 -> sha256 because 512 is big

718fdfb

WIP updated goldens

d809fc2

WIP renamed startScreenshotTask_; removed references to "ID" for "hash"

b9888b0

lynnmercier approved these changes Mar 22, 2018

View reviewed changes

patrickrodee added 3 commits March 22, 2018 16:43

WIP ✌️ to all-caps constants and switch statements

81a1b13

WIP sorted private methods

667a1b6

WIP made my tests work for real

aae3ef4

acdvorak reviewed Mar 23, 2018

View reviewed changes

test/screenshot/upload-screenshots.js

@@ -1,59 +0,0 @@

/**

Copy link

acdvorak Mar 23, 2018

Choose a reason for hiding this comment

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

Don't forget to remove or update npm run upload:screenshots in package.json

acdvorak approved these changes Mar 23, 2018

View reviewed changes

moog16 merged commit 98bcdfe into master Mar 30, 2018

moog16 deleted the experimental/gcp-upload branch July 17, 2018 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: remote hosted screenshot testing #12

feat: remote hosted screenshot testing #12

patrickrodee commented Mar 22, 2018

acdvorak left a comment

acdvorak Mar 22, 2018

acdvorak Mar 22, 2018

acdvorak Mar 22, 2018

acdvorak Mar 22, 2018

acdvorak Mar 22, 2018

patrickrodee Mar 22, 2018

acdvorak Mar 22, 2018

acdvorak Mar 22, 2018

acdvorak Mar 22, 2018

patrickrodee Mar 22, 2018

acdvorak Mar 22, 2018

acdvorak Mar 22, 2018

lynnmercier Mar 22, 2018

acdvorak Mar 22, 2018

lynnmercier Mar 22, 2018

lynnmercier Mar 22, 2018

acdvorak Mar 23, 2018

acdvorak Mar 23, 2018

acdvorak Mar 23, 2018

acdvorak Mar 23, 2018

acdvorak left a comment

feat: remote hosted screenshot testing #12

feat: remote hosted screenshot testing #12

Conversation

patrickrodee commented Mar 22, 2018

acdvorak 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

acdvorak left a comment

Choose a reason for hiding this comment