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

feat(infrastructure): Upload compiled screenshot test assets to GCS #2500

Merged
merged 10 commits into from
Apr 4, 2018

Conversation

acdvorak
Copy link
Contributor

@acdvorak acdvorak commented Apr 3, 2018

Resolves #1996

New NPM command: screenshot:upload-assets.

First, run:

npm start

Then, in another terminal, run:

export MDC_GCLOUD_SERVICE_ACCOUNT_KEY_FILE_PATH=path/to/gcloud-service-account-key.json
npm run screenshot:upload-assets

To avoid collisions between developers, every batch of files is uploaded to a directory with the following format:

/<bucket>/<username>/<timestamp>/<commit>/assets/

E.g.: /mdc-web-screenshot-tests/advorak/2018/04/03/23_00_32_993/5ca514fe/assets/.

Sample output:

New NPM command: `screenshot:upload-assets`.

First, run:

```bash
npm start
```

Then, in another terminal, run:

```bash
npm run screenshot:upload-assets
```

All files are uploaded to a directory containing a random UUID to prevent collisions between developers.
@codecov-io
Copy link

codecov-io commented Apr 3, 2018

Codecov Report

Merging #2500 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2500   +/-   ##
=======================================
  Coverage   98.82%   98.82%           
=======================================
  Files         102      102           
  Lines        4079     4079           
  Branches      511      511           
=======================================
  Hits         4031     4031           
  Misses         48       48

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10a75f6...f31324f. Read the comment docs.

Copy link
Contributor

@patrickrodee patrickrodee left a comment

Choose a reason for hiding this comment

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

Looks dope. Got a few comments.

const relativeLocalFilePaths = glob.sync(path.join(LOCAL_DIRECTORY_PREFIX, '**/*.*'));

relativeLocalFilePaths.forEach((relativeLocalFilePath) => {
const fileContents = fs.readFileSync(relativeLocalFilePath);
Copy link
Contributor

Choose a reason for hiding this comment

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

readFileSync to avoid having too many open files at once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's to ensure that upload() doesn't immediately return an empty array.

I tried several different approaches first, but they were all more convoluted and harder to read, so I went with readFileSync() cuz it was the simplest.

}

function handleUploadFailure(absoluteGcsBaseDir, relativeGcsFilePath, err) {
console.error(`❌︎ FAILED to upload ${GCLOUD_STORAGE_BASE_URL}${absoluteGcsBaseDir}${relativeGcsFilePath}:\n`, err);
Copy link
Contributor

Choose a reason for hiding this comment

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

What icon is this? I only see a square.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Changed to ✗ (Ballot X)

console.log('\n\nDONE!\n\n');
console.log(htmlFileUrls.join('\n'));
},
(err) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Use .catch instead. It's 100% a personal preference but I find it much more obvious what the function is doing.

Copy link
Contributor Author

@acdvorak acdvorak Apr 3, 2018

Choose a reason for hiding this comment

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

Good idea! Done.

Copy link
Contributor

@patrickrodee patrickrodee left a comment

Choose a reason for hiding this comment

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

LGTM

promises.push(
file.save(fileContents, fileOptions).then(
() => handleUploadSuccess(baseGcsDir, relativeGcsFilePath),
(err) => handleUploadFailure(baseGcsDir, relativeGcsFilePath, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

SUPER NIT: use .catch here, too. Gonna LGTM this without because this is just me being obscenely picky.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Done.

@acdvorak acdvorak merged commit 5ada5b4 into master Apr 4, 2018
@acdvorak acdvorak deleted the feat/test/upload-assets-to-gcs branch April 4, 2018 00:17
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.

3 participants