feat(screenshot): Improve Firebase Functions for screenshot tests #3732

tinayuangao · 2017-03-23T01:10:10Z

Based on feat(screenshot): Use firebase functions and jwt to make screenshot tests more secure #3628
Fixes Screenshot Testing - Firebase Functions #3686
Use typescript. Put functions in seperate files.
Add function to update Github Status based on result
Add function to approve a PR (copy test images to goldens)
Remove functions to upload filenames (We don't need it anymore)

devversion · 2017-03-24T23:38:48Z

@tinayuangao Should I review this once #3628 is merged?

tinayuangao · 2017-03-24T23:56:01Z

@devversion Yes. I will remove in-progress label when it's ready.

devversion · 2017-03-25T08:07:24Z

functions/data_image.ts

+
+/**
+ * Convert data to images. Image data posted to database will be saved as png files
+ * and upload to screenshot/$prNumber/dataType/$filename


uploaded?

devversion · 2017-03-25T08:09:02Z

functions/data_image.ts

+  let data = event.data.val();
+  let saveFilename = `${event.params.filename}.screenshot.png`;
+
+  if (dataType != 'diff' && dataType != 'test') {


Nit: It is always desired to use strict comparison - dataType !== 'diff'

devversion · 2017-03-25T08:11:56Z

functions/github.ts

+  let result = event.data.val() == true;
+  let prNumber = event.params.prNumber;
+  return event.data.ref.parent.child('sha').once('value').then((sha: firebaseAdmin.database.DataSnapshot) => {
+    return setGithubStatus(sha.val(),


I think the API we used in /tools/gulp/util/github is a bit more convenient. You just pass in a JSON config instead of X parameters.

devversion · 2017-03-25T08:24:05Z

functions/package.json

-    "jsonwebtoken": "^7.3.0"
+    "jsonwebtoken": "^7.3.0",
+    "request": "^2.81.0",
+    "typescript": "^2.2.1"


I thought we build the functions/*.ts file before deploying and Firebase shouldn't care about it? If so then we can just remove it from the package file here.

devversion · 2017-03-25T08:24:53Z

functions/test_goldens.ts

+      let keys: string[] = [];
+      let counter = 0;
+      snapshot.forEach((childSnapshot: firebaseAdmin.database.DataSnapshot) => {
+        if (childSnapshot.val() == false) {


Nit: Strict comparsion

devversion · 2017-03-25T08:33:56Z

functions/test_goldens.ts

+          keys.push(childSnapshot.key);
+        }
+        counter ++;
+        if (counter == snapshot.numChildren()) return true;


Why do we need to manually cancel the enumeration? Doesn't the loop stop automatically when you iterated through all?

It is.
But typescript requires return boolean, and once boolean returned the forEach loop will end.

devversion · 2017-03-25T08:34:15Z

functions/test_goldens.ts

+export function copyTestImagesToGoldens(prNumber: string) {
+  return firebaseAdmin.database().ref(`screenshot/reports/${prNumber}/results`).once('value')
+    .then((snapshot: firebaseAdmin.database.DataSnapshot) => {
+      let keys: string[] = [];


How about failedFilenames?

devversion · 2017-03-25T08:34:55Z

functions/tsconfig.json

+    "jwt_util.ts",
+    "test_goldens.ts",
+    "util/github.ts",
+    "util/jwt.ts"


Assuming that index.ts is your entry-point and all other files are somehow imported by it. You could just add index.ts as file her.e

devversion · 2017-03-25T08:35:34Z

functions/util/github.ts

+                                description: string,
+                                url: string,
+                                repoSlug: string,
+                                token: string) {


As mentioned above, I would prefer the syntax / API we currently have on tools/gulp/util/github.ts

devversion · 2017-03-25T08:36:54Z

tools/gulp/tasks/screenshots.ts

      .then(() => database.goOffline(), () => database.goOffline());
  }
 });

 function updateFileResult(database: firebase.database.Database, prNumber: string,
                          filenameKey: string, result: boolean) {
-  return getPullRequestRef(database, prNumber).child('results').child(filenameKey).set(result);
+  return getPullRequestRef(database, prNumber).child('results').child(filenameKey).set(result)  ;


Unused extra space at line end?

tinayuangao · 2017-03-27T21:55:48Z

Comments addressed. Please take another look. Thanks!

jelbourn · 2017-03-28T01:29:55Z

functions/data.ts

+ * temporary folder if the token is valid.
+ * Move the data to 'screenshot/reports/$prNumber/$path
+ */
+export function verifyJWTAndUpdateData(event: any, path: string) {


verifyJwtAndTransferResultToTrustedLocation?

jelbourn · 2017-03-28T01:33:21Z

functions/data.ts

@@ -0,0 +1,22 @@
+import * as firebaseAdmin from 'firebase-admin';


Could there be a more specific name for this file than data.ts? Maybe verify-and-copy-screenshot-report.ts or copy-trusted-screenshot-report.ts?

In general the screenshot-related files under functions/ should have screenshot in the name somewhere (since we're likely going to add more firebase functions). A screenshot-test/ subdirectory would also work if firebase supports that.

jelbourn · 2017-03-28T01:40:32Z

functions/data.ts

+ * Handle data written to temporary folder. Validate the JWT and move the data out of
+ * temporary folder if the token is valid.
+ * Move the data to 'screenshot/reports/$prNumber/$path
+ */


/** * Verifies that a screenshot report is valid (trusted via JWT) and, if so, copies it from the * temporary, unauthenticated location to the more permanent, trusted location. */

jelbourn · 2017-03-28T01:46:09Z

functions/data.ts

+ */
+export function verifyJWTAndUpdateData(event: any, path: string) {
+  // Only edit data when it is first created. Exit when the data is deleted.
+  if (event.data.previous.exists() || !event.data.exists()) {


Would it make sense to create utility functions for things like this? E.g.,

isEditEvent(event) { return event.data.previous.exists() && event.data.exists(); } isDeleteEvent(event) { return event.data.previous.exists() && !event.data.exists(); } isCreateEvent(event) { return !event.data.previous.exists() && event.data.exists(); }

jelbourn · 2017-03-28T01:51:48Z

functions/jwt_util.ts

+ * Verify event params have correct JsonWebToken, and execute callback when the JWT is verified.
+ * Delete the data if there's an error or the callback is done
+ */
+export function verifySecureTokenAndExecute(event: firebaseFunctions.Event<any>) {


Since this returns a promise, I would just call the function verifySecureToken. That way, then calling it, it will read like

"Verify secure token, then ..."

jelbourn · 2017-03-28T02:07:57Z

functions/data.ts

+    return firebaseAdmin.database().ref().child('screenshot/reports')
+      .child(prNumber).child(path).set(data);
+  });
+};


How much of the overall screenshot code could be moved to tools/screenshot-testing and how much needs to live in functions/? I'd like to try to package everything under tools/ as the shareable package that can be used in other repos. Ideally the bits that live in functions/ would be very minimal.

jelbourn · 2017-03-28T02:08:50Z

functions/data_image.ts

+ * Convert data to images. Image data posted to database will be saved as png files
+ * and uploaded to screenshot/$prNumber/dataType/$filename
+ */
+export function convertTestImageDataToFiles(event: any) {


"Convert data to images"

What format is the data that's being converted?

jelbourn · 2017-03-28T02:09:21Z

functions/data_image.ts

+  let data = event.data.val();
+  let saveFilename = `${event.params.filename}.screenshot.png`;
+
+  if (dataType !== 'diff' && dataType !== 'test') {


Add a comment explaining what this condition is checking for?

jelbourn · 2017-03-28T02:14:06Z

functions/index.js

-const jwt = require('jsonwebtoken');
-const fs = require('fs');
-
+Object.defineProperty(exports, "__esModule", { value: true });


What's going on with these imports? At the very least it needs an explanatory comment.

I think that file was accidentally committed? There is also the index.ts. @tinayuangao

jelbourn · 2017-03-28T02:15:54Z

functions/index.js

+var image_data_1 = require("./image_data");
+var data_image_1 = require("./data_image");
+var test_goldens_1 = require("./test_goldens");
+var github_1 = require("./github");


Prefer single-quotes over double-quotes.

devversion · 2017-04-03T17:55:20Z

.gitignore

@@ -8,6 +8,8 @@

 # dependencies
 /node_modules
+/functions/node_modules
+/tools/screenshot-test/functions/node_modules


If you remove the slash for the /node_modules above. Then it will include all node_modules/ folders in the whole project.

Just do

node_modules/

jelbourn · 2017-04-06T15:55:07Z

tools/gulp/tasks/screenshots.ts

-  if (prNumber) {
+
+  if (isTravisPushBuild()) {
+    // Only update goldens for build


// Only update goldens for commits to master

?

Also, it might make sense to rename isTravisPushBuild to isTravisMasterBuild (or invert the function and rename it to isTravisPullRequestBuild)

jelbourn · 2017-04-06T16:11:35Z

tools/screenshot-test/functions/data_image.ts

+const bucket = gcs.bucket(firebaseFunctions.config().firebase.storageBucket);
+
+/**
+ * Convert data to images. Image data posted to database will be saved as png files


Is the incoming data a base-64 encoded image? If so, the comment should say something like

"Writes base-64 encoded test images to png files on the filesystem."

I'd also rename the function to writeTestImagesToFiles

jelbourn · 2017-04-06T16:14:09Z

tools/screenshot-test/functions/data_image.ts

@@ -0,0 +1,39 @@
+import * as firebaseFunctions from 'firebase-functions';


Can you use dashes in the filenames instead of underscores?

I would also call this file write-images.ts

jelbourn · 2017-04-06T16:16:48Z

tools/screenshot-test/functions/image_data.ts

+/**
+ * Read golden files under /goldens/ and store the image data to
+ * database /screenshot/goldens/$filename
+ * Convert png image files to BufferArray data


Is this a base-64 encoded string?

jelbourn · 2017-04-06T16:18:21Z

tools/screenshot-test/functions/image_data.ts

+ * database /screenshot/goldens/$filename
+ * Convert png image files to BufferArray data
+ */
+export function convertGoldenImagesToData(name: string, resourceState: string, fileBucket: any) {


Maybe copyGoldImagesToDatabase?

jelbourn · 2017-04-06T16:23:13Z

tools/screenshot-test/functions/test_goldens.ts

+        }
+        counter++;
+        if (counter == snapshot.numChildren()) {
+          return true;


I don't think this true is used

Here the type check requires return a boolean.

jelbourn · 2017-04-06T16:24:02Z

tools/screenshot-test/functions/test_goldens.ts

+      });
+      return failedFilenames;
+    }).then((failedFilenames: string[]) => {
+      return bucket.getFiles({prefix: `screenshots/${prNumber}/test`}).then(function (data: any) {


Arrow function here?

jelbourn · 2017-04-11T22:12:48Z

LGTM

angular-automatic-lock-bot · 2019-09-06T11:02:29Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 23, 2017

tinayuangao requested review from jelbourn and devversion March 23, 2017 19:14

tinayuangao assigned jelbourn and devversion Mar 23, 2017

tinayuangao force-pushed the screenshot-functions2 branch 2 times, most recently from 8bcbdd1 to 675197f Compare March 24, 2017 23:04

tinayuangao force-pushed the screenshot-functions2 branch from 675197f to 16ee200 Compare March 24, 2017 23:44

tinayuangao added the in progress This issue is currently in progress label Mar 24, 2017

tinayuangao force-pushed the screenshot-functions2 branch 3 times, most recently from 1f67986 to dd86b5d Compare March 25, 2017 01:26

tinayuangao added pr: needs review and removed in progress This issue is currently in progress labels Mar 25, 2017

devversion reviewed Mar 25, 2017

View reviewed changes

jelbourn reviewed Mar 28, 2017

View reviewed changes

devversion reviewed Apr 3, 2017

View reviewed changes

tinayuangao force-pushed the screenshot-functions2 branch 2 times, most recently from fffb5f0 to 62b56a3 Compare April 3, 2017 18:57

jelbourn reviewed Apr 6, 2017

View reviewed changes

tinayuangao force-pushed the screenshot-functions2 branch 2 times, most recently from 3d4887a to f1edc9e Compare April 10, 2017 18:31

tinayuangao added 5 commits April 10, 2017 17:05

Add approve functions and use typescript.

10ed932

Address comments

0ad5540

Add sha to result

2354542

Move files to tools/screenshot-test/functions

0c76b58

Address comments

5352b28

fix tslint

45dd431

tinayuangao force-pushed the screenshot-functions2 branch 2 times, most recently from 8c970fc to abe107d Compare April 11, 2017 00:17

try to fix closure compiler

c2e8cbc

tinayuangao force-pushed the screenshot-functions2 branch from abe107d to c2e8cbc Compare April 11, 2017 00:20

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Apr 11, 2017

jelbourn merged commit 25a6881 into angular:master Apr 17, 2017

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

		@@ -0,0 +1,22 @@
		import * as firebaseAdmin from 'firebase-admin';

		@@ -0,0 +1,39 @@
		import * as firebaseFunctions from 'firebase-functions';

feat(screenshot): Improve Firebase Functions for screenshot tests #3732

feat(screenshot): Improve Firebase Functions for screenshot tests #3732

Conversation

tinayuangao commented Mar 23, 2017

devversion commented Mar 24, 2017

tinayuangao commented Mar 24, 2017

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

tinayuangao commented Mar 27, 2017

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

jelbourn commented Apr 11, 2017

angular-automatic-lock-bot bot commented Sep 6, 2019