-
Notifications
You must be signed in to change notification settings - Fork 178
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
test(labware-library): add cypress e2e tests to the labware creator #4800
Conversation
Codecov Report
@@ Coverage Diff @@
## edge #4800 +/- ##
==========================================
- Coverage 57.95% 57.92% -0.03%
==========================================
Files 956 956
Lines 26212 26237 +25
==========================================
+ Hits 15191 15199 +8
- Misses 11021 11038 +17
Continue to review full report at Codecov.
|
bf56a89
to
2f47a44
Compare
…re creator Added cypress and cypress-file-upload npm modules to the project. Configured jest to ignore cypress tests. Added 'make test-e2e' task to the labware-library application. Created cypress directory structure and configi to the labware-library. Created a number of integration tests for the labware-creator sub-application. test(labware creator): disabled no-undef lint rule for new cypress.io tests
2f47a44
to
4ca9f6b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! A few minor things to get this ready to merge:
- We should add e2e testing instructions to
CONTRIBUTING.md
- We should update the eslint configuration to support cypress
- I can't find anything recommending committing video artifacts; can you either point me to such a recommendation or remove them?
- If possible, I think we'd prefer arrow functions in our
describe
etc blocks so we don't have to switch back and forth when writing unit and e2e tests
…back style changes eslint configuration, integrate server with test make command, remove test run movies, add testing instructions to contributing document
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, should've posted a more explicit example of what I meant by updating the eslint overrides. Otherwise this looks great!
@@ -194,6 +195,10 @@ make test | |||
# run tests per language | |||
make test-py | |||
make test-js | |||
|
|||
# run cypress tests for the labware-library | |||
cd labware-library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally we prefer make -C directory task
to explicit cd
s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was basing my addition on this from the protocol-designer readme:
# from the repo root
# install all dependencies
make install
cd protocol-designer/
# run the development server
make dev
The next round of additions will include protocol-designer tests, and I'll want to refactor how the tests get run, I think, since they'll be in multiple top-level applications. I'll revisit this then.
.eslintrc.js
Outdated
@@ -13,7 +13,14 @@ module.exports = { | |||
'prettier/standard', | |||
], | |||
|
|||
plugins: ['flowtype', 'react', 'react-hooks', 'json', 'prettier'], | |||
plugins: [ | |||
// 'eslint-plugin-cypress', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover unnecessary comment (this plugin is enabled by the recommended plugin config in the override block in the appropriate files)
.eslintrc.js
Outdated
@@ -35,6 +42,7 @@ module.exports = { | |||
env: { | |||
node: true, | |||
browser: true, | |||
// 'cypress/globals': true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also leftover unnecessary comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh! I rebased my commit and got rid of those.
0a10f78
to
5cb6582
Compare
5cb6582
to
a09ab0a
Compare
overview
This PR adds cypress.io infrastructure to the repo. Cypress.io is an e2e testing framework, similar to selenium but better suited for modern web applications. The first application to get cypress tests is the labware creator.
Since my time here is limited (that is, I will not be maintaining these tests), I opted for simplicity and minimal code abstraction. There is opportunity to create more general functions used in all the test runs I've made here. Cypress generates movies of all of the test runs, and I've included those in my commit.
Besides testing every permutation of custom labware, it also tests importing existing labware definitions for editing and re-export. All of these tests together run in just over two minutes. The next step for these tests could be to integrate them with the build/CI process so they get run automatically.
changelog
review requests
The only change to existing functionality is adding a rule to jest to skip the cypress tests when it run the unit tests. Everything else sits on top of what's already here.