ci(travis): run cypress e2e tests on build #5431

IanLondon · 2020-04-14T20:31:19Z

overview

Run cypress tests in Travis! Build will fail if any e2e tests fail. Closes #5318

This is dependent on #5424 - after it is merged, I'll rebase this PR and change if (global.Cypress) in that PR to if (process.env.CYPRESS === '1').

changelog

review requests

Sanity check!

One weird thing I encountered was that labware library tests failed in Travis because GTM_ID env var was set, which somehow caused an error to be thrown about Intercom being undefined. So I set GTM_ID='' and also just to be safe I cut out the GTM tag from the Handlebars HTML template if GTM_ID is falsey.
I'm using wait-until to make sure that the dev server is running before trying to start Cypress
Cypress by default saves videos. I don't think they're useful to us, so I turned them off. We can always enable them and upload them to S3 if we decide that's good, but otherwise it just slows down the build.
I didn't add any Make commands for running Cypress while you're writing tests. I kind of don't want to, I'd rather do cd protocol-designer; yarn cypress open (whatever args) rather than going thru Make.

risk assessment

Significant risk to our workflow, but easy to roll back. If this causes builds to fail intermittently, or go too slowly, we'll have to change it back. Worst case scenario is removing make test-e2e from Travis yml to stop running the tests until we fix whatever problem we might have.

codecov · 2020-04-14T20:31:26Z

Codecov Report

Merging #5431 into edge will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             edge    #5431   +/-   ##
=======================================
  Coverage   65.64%   65.64%           
=======================================
  Files        1084     1084           
  Lines       30905    30905           
=======================================
  Hits        20287    20287           
  Misses      10618    10618

Impacted Files	Coverage Δ
...designer/src/components/FileSidebar/FileSidebar.js	`77.77% <100.00%> (ø)`

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 b45af1e...42a5716. Read the comment docs.

dozercodes · 2020-04-15T15:27:33Z

protocol-designer/src/components/FileSidebar/FileSidebar.js

@@ -43,7 +43,7 @@ const saveFile = (downloadData: $PropertyType<Props, 'downloadData'>) => {
  const blob = new Blob([JSON.stringify(downloadData.fileData)], {
    type: 'application/json',
  })
-  if (global.Cypress) {
+  if (process.env.CYPRESS === '1') {


This will also have to be changed to an env var in your other PR #5424, right?

mcous

If possible, I would love these e2e tests to be parallel rather than balloon out the existing JS test job

mcous · 2020-04-16T16:35:48Z

.travis.yml

@@ -84,13 +84,14 @@ jobs:

    # test, build, and upload for JavaScript projects
    - stage: test
-      name: 'JS unit tests; build Protocol Designer, Labware Library, Components Library'
+      name: 'JS unit & E2E tests; build Protocol Designer, Labware Library, Components Library'


I think it's a better idea to put E2E tests in their own parallel job in the test stage rather than blowing this job out even further.

If the reason it was placed in this stage was to prevent deploy of PD if the e2e tests don't pass, then I have two thoughts while we're still on Travis:

We could move PD deploy to a job in the "deploy" stage

This gates PD deploy on API tests passing

We could continue to put PD artifacts in S3 regardless of what the e2e tests say

PD "deploy" uploads artifacts in S3; it does not actually deploy anything

I don't think preventing artifact uploads on e2e failures is necessarily worth the effort (not that it's a bad idea or anything)

don't think preventing artifact uploads on e2e failures is necessarily worth the effort (not that it's a bad idea or anything)

👍

mcous · 2020-04-16T16:38:22Z

labware-library/Makefile

-		"$(MAKE) dev" \
-		"cypress run --browser chrome"
+	concurrently --no-color --kill-others --success first --names "labware-library-server,labware-library-tests" \
+	"$(MAKE) dev CYPRESS=1 GTM_ID=''" \


Why are we defining a new environment variable? We already have NODE_ENV=test, I feel like it might be a little more straightforward to do NODE_ENV=cypress or e2e or something similar

It looks like we only ever set NODE_ENV to 'production' or 'development', I don't see test?

I was worried that since we're just using 2 states of NODE_ENV, we'd find places in our code with an assumption "if NODE_ENV isn't production then it must be development". But now going through uses of NODE_ENV the only time I see something like that is const DEV_MODE = process.env.NODE_ENV !== 'production' in app-shell which isn't going to cause an issue.

However, it's easy to do if (NODE_ENV === 'development ) {/* dev stuff */} else {/* prod stuff, but oops this also happens when NODE_ENV is cypress */} and IMO this is more complicated to think about than using a new env var.

Also any use of process.env.CYPRESS env var in the app code is an easy-to-search-for indicator that we had to do something gross to get Cypress to do something that it's not able to do, like looking at saved files

agree re:

easy-to-search-for indicator

But I think we should be in the habit of explicitly checking strings rather than relying on an else

It looks like we only ever set NODE_ENV to 'production' or 'development', I don't see test?

I don't much care, but since Jest sets NODE_ENV to test during testing, (see https://jestjs.io/docs/en/getting-started#using-babel; expand the "Making your Babel config jest-aware" section) I suggested NODE_ENV="cypress" to match that behavior. You can see our usage of this in our babel config:

env: { // Note(isk: 3/2/20): Must have babel-plugin-styled-components in each env, // see here for further details: s https://styled-components.com/docs/tooling#babel-plugin production: { plugins: ['babel-plugin-styled-components', 'babel-plugin-unassert'], }, development: { plugins: ['babel-plugin-styled-components', 'react-hot-loader/babel'], }, test: { plugins: [ 'babel-plugin-styled-components', '@babel/plugin-transform-modules-commonjs', 'babel-plugin-dynamic-import-node', ], }, },

However, it's easy to do if (NODE_ENV === 'development' ) {/* dev stuff /} else {/ prod stuff, but oops this also happens when NODE_ENV is cypress */} and IMO this is more complicated to think about than using a new env var.

In my experience, checking for production environment should always be checking that the env was explicitly set to production to avoid this exact issue.

mcous · 2020-04-16T16:44:58Z

protocol-designer/src/components/FileSidebar/FileSidebar.js

@@ -43,7 +43,7 @@ const saveFile = (downloadData: $PropertyType<Props, 'downloadData'>) => {
  const blob = new Blob([JSON.stringify(downloadData.fileData)], {
    type: 'application/json',
  })
-  if (global.Cypress) {
+  if (process.env.CYPRESS === '1') {
    // HACK(IL, 2020-04-02): can't figure out a better way to do this yet


Rather than polluting the application code here, could you replace fileSaver using webpack or babel if the environment doesn't support it? See eligrey/FileSaver.js#501

Also, the app attached the Redux store to the window if the NODE_ENV isn't production, but that doesn't feel like the problem here

It will save fine under Cypress, but Cypress has no way to locate the file. Sometimes the test will timeout if a file is saved because depending on browser settings it might make a native "Save File..." modal that never closes, but that doesn't have to do with fileSaver

Yeah to be clear, I'm not suggesting we change this global.__lastSavedFile__ = downloadData.fileData behavior during Cypress runs. I'm saying that this override should happen using a mock for FileSaver that webpack (using the resolve) config or babel (using babel-plugin-module-resolver) shims in when the environment is Cypress rather than putting the override logic in application code

Regardless, certainly not a request for this PR, sorry for not making clear in the original post

dozercodes

🌲

Kadee80

🌵

shlokamin

Good stuff @IanLondon!

shlokamin · 2020-04-21T16:36:41Z

labware-library/Makefile

-		"$(MAKE) dev" \
-		"cypress run --browser chrome"
+	concurrently --no-color --kill-others --success first --names "labware-library-server,labware-library-tests" \
+	"$(MAKE) dev CYPRESS=1 GTM_ID=''" \


agree re:

easy-to-search-for indicator

But I think we should be in the habit of explicitly checking strings rather than relying on an else

mcous

Posted some clarifications but this LGTM. Looking over the Travis run is making me more convinced we should switch to GH actions and get even more parallelization going on

mcous · 2020-04-21T17:46:35Z

.travis.yml

@@ -84,7 +84,7 @@ jobs:

    # test, build, and upload for JavaScript projects
    - stage: test
-      name: 'JS unit tests; build Protocol Designer, Labware Library, Components Library'
+      name: 'JS unit; build Protocol Designer, Labware Library, Components Library'


Unintentional removal of "tests"?

mcous · 2020-04-21T17:51:55Z

protocol-designer/src/components/FileSidebar/FileSidebar.js

@@ -43,7 +43,7 @@ const saveFile = (downloadData: $PropertyType<Props, 'downloadData'>) => {
  const blob = new Blob([JSON.stringify(downloadData.fileData)], {
    type: 'application/json',
  })
-  if (global.Cypress) {
+  if (process.env.CYPRESS === '1') {
    // HACK(IL, 2020-04-02): can't figure out a better way to do this yet


Yeah to be clear, I'm not suggesting we change this global.__lastSavedFile__ = downloadData.fileData behavior during Cypress runs. I'm saying that this override should happen using a mock for FileSaver that webpack (using the resolve) config or babel (using babel-plugin-module-resolver) shims in when the environment is Cypress rather than putting the override logic in application code

Regardless, certainly not a request for this PR, sorry for not making clear in the original post

…not the one listed first Xo

* run cypress tests in new Travis job * labware-library: do not add GTM script when E2E testing

IanLondon added ready for review infrastructure ops/ CI/ repo etc labels Apr 14, 2020

IanLondon requested review from mcous, sfoster1, Kadee80, b-cooper and shlokamin April 14, 2020 20:31

IanLondon requested a review from a team as a code owner April 14, 2020 20:31

dozercodes reviewed Apr 15, 2020

View reviewed changes

mcous added the robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). label Apr 15, 2020

mcous reviewed Apr 16, 2020

View reviewed changes

IanLondon force-pushed the ci_add-cypress-to-travis branch from 71897c5 to 0ab1098 Compare April 20, 2020 19:58

dozercodes approved these changes Apr 21, 2020

View reviewed changes

Kadee80 approved these changes Apr 21, 2020

View reviewed changes

shlokamin approved these changes Apr 21, 2020

View reviewed changes

mcous approved these changes Apr 21, 2020

View reviewed changes

IanLondon added 5 commits April 21, 2020 14:05

ci(travis) add cypress tests to travis

22175e7

do not add GTM when testing

ba3cec3

use --success first, b/c it references the process that exits first, …

76ec203

…not the one listed first Xo

move e2e tests to new Travis job

8eee7ee

typo

42a5716

IanLondon force-pushed the ci_add-cypress-to-travis branch from 0ab1098 to 42a5716 Compare April 21, 2020 18:08

IanLondon merged commit a31331d into edge Apr 21, 2020

IanLondon deleted the ci_add-cypress-to-travis branch April 21, 2020 19:08

IanLondon added a commit that referenced this pull request Apr 22, 2020

ci(travis): run cypress e2e tests on build (#5431)

0afc3c4

* run cypress tests in new Travis job * labware-library: do not add GTM script when E2E testing

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci(travis): run cypress e2e tests on build #5431

ci(travis): run cypress e2e tests on build #5431

IanLondon commented Apr 14, 2020 •

edited

Loading

codecov bot commented Apr 14, 2020 •

edited

Loading

dozercodes Apr 15, 2020

mcous left a comment

mcous Apr 16, 2020

IanLondon Apr 20, 2020

mcous Apr 16, 2020

IanLondon Apr 20, 2020

shlokamin Apr 21, 2020

mcous Apr 21, 2020

mcous Apr 16, 2020

IanLondon Apr 20, 2020

mcous Apr 21, 2020 •

edited

Loading

dozercodes left a comment

Kadee80 left a comment

shlokamin left a comment

shlokamin Apr 21, 2020

mcous left a comment

mcous Apr 21, 2020

IanLondon Apr 21, 2020

mcous Apr 21, 2020 •

edited

Loading

ci(travis): run cypress e2e tests on build #5431

ci(travis): run cypress e2e tests on build #5431

Conversation

IanLondon commented Apr 14, 2020 • edited Loading

overview

changelog

review requests

risk assessment

codecov bot commented Apr 14, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

mcous 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

mcous Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

dozercodes left a comment

Choose a reason for hiding this comment

Kadee80 left a comment

Choose a reason for hiding this comment

shlokamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcous left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcous Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

IanLondon commented Apr 14, 2020 •

edited

Loading

codecov bot commented Apr 14, 2020 •

edited

Loading

mcous Apr 21, 2020 •

edited

Loading

mcous Apr 21, 2020 •

edited

Loading