chore(cypress): upgrade cypress to v8 and turn on tests #2063
Conversation
|
github-actions
bot
added
Area: Infrastructure
|
✔️ Deploy Preview for paste-docs ready! 🔨 Explore the source changes: 9f68dce 🔍 Inspect the deploy log: https://app.netlify.com/sites/paste-docs/deploys/619d34f5a9972500089c5850 😎 Browse the preview: https://deploy-preview-2063--paste-docs.netlify.app |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 9f68dce:
|
|
Test summary
Run details
View run in Cypress Dashboard ➡️ Failures
Flakiness
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
||||||||||||||||||||||||||||||||||
|
✔️ Deploy Preview for paste-theme-designer ready! 🔨 Explore the source changes: 9f68dce 🔍 Inspect the deploy log: https://app.netlify.com/sites/paste-theme-designer/deploys/619d34f5c0909a00077b8af2 😎 Browse the preview: https://deploy-preview-2063--paste-theme-designer.netlify.app |
|
Size Change: 0 B Total Size: 461 kB ℹ️ View Unchanged
|
github-actions
bot
added
the
Area: Components
andioneto
force-pushed
the
chore/upgrade-cypress
branch
from
64e0bb1 to
d59483b
Compare
github-actions
bot
removed
the
Area: Doc Site
andioneto
force-pushed
the
chore/upgrade-cypress
branch
from
e84ca48 to
a2bbe83
Compare
github-actions
bot
removed
the
Area: Components
andioneto
changed the title
|
For visibility I want to surface this comment from the now closed PR #2056 (comment)
I too have concerns about testing every page with the same duplicate tests. What mechanism is in place to make sure new pages get new tests? is it written down? Is there some kind of check? Human error will be high on this and pages will get missed |
My thinking was that it will be pretty easy to add the script to compare documented pages to tested pages and run the generation on some interval. That is not currently implemented and I have not yet documented all this, but it is still technically a work in progress.
In an ideal world where all of our website pages constructed using the same utilities (e.g components etc) that makes sense. In it's current state though, that's not really the case and we are leaving a lot of room for human error in the source code. Because of that I think there is a benefit to testing pages individually. Also, each page has different data, which could leave us vulnerable to bugs as we modify/update the website pages. |
andioneto
force-pushed
the
chore/upgrade-cypress
branch
from
d503a71 to
cd40f21
Compare
| buildScriptName: "build:storybook" | ||
| autoAcceptChanges: "main" | ||
| exitOnceUploaded: true | ||
| # react16_tests: |
There was a problem hiding this comment.
Temporarily commented-out these workflow jobs so as to not over-run them on GitHub Actions. Should be un-commented before this PR is merged.
|
I don't think we're quite there as we're tripling the build minutes, where each job is building the website. It's about 25 minutes of website building. Can we share the website build artefacts?
|
Funny you should ask, I have a branch where I added a job to build the website after the other assets are built, and the tests just load them from the cache. |
andioneto
force-pushed
the
chore/upgrade-cypress
branch
from
735ca64 to
9f68dce
Compare
| @@ -7,5 +7,6 @@ | |||
| "runMode": 2, | |||
| "openMode": 0 | |||
| }, | |||
| "blockHosts": "*.google-analytics.com" | |||
| "blockHosts": "*.google-analytics.com", | |||
| "nodeVersion": "system" | |||
There was a problem hiding this comment.
ℹ️ The default sets the node version to the package that is bundled with Cypress. See here for more info
This means that we were not necessarily running our tests with Node v12 as specified in our workflow. This instructs Cypress to use the version on the CI machine(s) (or local machine).
There was a problem hiding this comment.
Those docs state the default value for nodeVersion is system. You're just resetting the value to be the default value. You can remove it
There was a problem hiding this comment.
Also states its deprecated and will be removed
There was a problem hiding this comment.
@SiTaggart I'm going to remove this for now, but FWIW added this line to print the node version cypress is using it printed out the version that is bundled with the cypress version.
I wonder if the above snippet from the docs is true for more recent versions of cypress, but we are currently a few major version behind.
| @@ -59,58 +59,11 @@ jobs: | |||
| packages/paste-design-tokens/types/**/* | |||
| packages/paste-codemods/tools/.cache/mappings.json | |||
|
|
|||
| type_check: | |||
| name: Type check | |||
| build_website: | |||
There was a problem hiding this comment.
ℹ️ Job that will build the website immediately after the other package assets are finished building. This will then be saved to the cache to be used by the test job later.
| - name: Install Dependencies | ||
| if: steps.yarn_cache_id.outputs.cache-hit != 'true' || steps.node_modules_cache_id.outputs.cache-hit != 'true' | ||
| run: yarn install --immutable | ||
| packages/paste-website/.cache/mappings.json |
There was a problem hiding this comment.
TBH not sure if we actually need this, but it threw an error when I omitted all of the .cache files.
There was a problem hiding this comment.
We need this file. I might refactor it later to be more clear, but this file needs to be commit and shipped.
There was a problem hiding this comment.
Yeah that sounds reasonable. FWIW this isn't being used to build the website on release, only for the pull requests CI.
| # make sure every Cypress install prints minimal information | ||
| CI: 1 | ||
| # print Cypress and OS info | ||
| run: | |
There was a problem hiding this comment.
ℹ️ This will verify the cypress installation before running our tests, as well as print some logs about the environment the tests will run in. This can be helpful to ensure that we are using the system config that we expect.
| AIRTABLE_BASEID: ${{ secrets.AIRTABLE_BASEID }} | ||
| GATSBY_DOCSEARCH_APIKEY: ${{ secrets.GATSBY_DOCSEARCH_APIKEY }} | ||
| # make sure every Cypress install prints minimal information | ||
| CI: 1 |
There was a problem hiding this comment.
ℹ️ CI flag that limits logging output.
| path: packages/ | ||
| name: compiled-website | ||
| path: packages/paste-website/ | ||
| if-no-files-found: error |
There was a problem hiding this comment.
ℹ️ I noticed that if the save-cache step fails it just throws a warning, and does not error out. This will cause the job to fail and the action to stop running if no files are found on download.
| # leaving the Dashboard hanging ... | ||
| # https://github.com/cypress-io/github-action/issues/48 | ||
| fail-fast: false | ||
| matrix: |
There was a problem hiding this comment.
ℹ️ this is a config specific to github actions to allow parallelization. Note that there are separate required configuration options for the cypress command itself (which are noted in a comment in the package.json file)
| @@ -151,60 +103,25 @@ jobs: | |||
| with: | |||
| name: compiled-js-and-types | |||
| path: packages/ | |||
| if-no-files-found: error | |||
There was a problem hiding this comment.
ℹ️ I noticed that if the save-cache step fails it just throws a warning, and does not error out. This will cause the job to fail and the action to stop running if no files are found on download. (also added to the job build_website
| @@ -70,7 +70,8 @@ | |||
| "test:tools": "jest ./tools/ -c ./tools/jest.config.js", | |||
| "test:tools-16": "USE_REACT_16=true jest ./tools/ -c ./tools/jest.config.js", | |||
| "test:coverage": "yarn pre-test && jest --coverage", | |||
| "test:website": "start-server-and-test 'yarn serve:website' http://localhost:9000 'yarn cypress run --record --spec ./cypress/integration/**/*.spec.ts'", | |||
| "test:website": "start-server-and-test 'yarn serve:website' http://localhost:9000 'yarn cypress run --record --parallel --group cypress-tests --spec \"./cypress/integration/**/**/*.spec.ts\"'", | |||
There was a problem hiding this comment.
ℹ️ Parallelized tests in cypress are available in a CI environment when:
- the run command has
--parallel=true - the run command has
--record=true - the CI environment config specifies the number of machines to run
Also note that we added a grouping label to this command.
It could be helpful in the future to split out our tests into separate jobs and group them by type/category.
🪙 🪙 The biggest value add here is that it could help us analyze our testing data in the future via the cypress dashboard & api.
| @@ -129,11 +131,32 @@ Cypress.Commands.add('checkInPageNavigationLinks', () => { | |||
| // @TODO Check ComponentHeader <--- waiting for changes to this component to be merged. | |||
| // Cypress.Commands.add('checkComponentHeader', () => {}); | |||
|
|
|||
| Cypress.Commands.add('abortPrefetchRequests', (target) => { | |||
There was a problem hiding this comment.
Do you mind explaining what this is doing?
There was a problem hiding this comment.
Oh yes, I totally forgot to add the docs to this command. Will add.
| xhr.xhr.abort(); | ||
| }, | ||
| }).as('appData'); | ||
| cy.route('http://api.github.com/repos/twilio-labs/paste', {}).as('githubData'); |
Just curious, what minor issues? Didn't see that documented anywhere.
Is this safe? This seems... sketchy. And very coupled to Gatsby, which we have hopes to migrate off of in the next year. How much faster do the tests run with/without this? Some perf numbers would help me better understand the evaluation of this trade off.
I couldn't grok this sentence :/ Could I get more words explaining what this means please?
I'm still seeing this PR fail on that and on the tabs page: |
| cy.get('[data-cy=patterns-button]').click({force: true}).should('have.attr', 'aria-expanded', 'true'); | ||
| cy.get('[data-cy=patterns-list]').should('be.visible'); | ||
| cy.get('[data-cy="patterns-button"]').click().shouldHaveAttribute('aria-expanded', 'true'); | ||
| cy.getInFixedContainer('[data-cy="patterns-list"]').should('be.visible'); |
There was a problem hiding this comment.
I don't recall these ever being flakey, and now they are. This leads me to wonder if this getInFixedContainer is the cause of flakiness.
| } | ||
| } | ||
|
|
||
| Cypress.Commands.add('getDocsPageContentArea', () => cy.get('#paste-docs-content-area')); | ||
|
|
||
| Cypress.Commands.add('pageHeaderShouldBeVisible', (headerText) => { | ||
| cy.contains('h1', headerText).shouldBeVisible(); | ||
| cy.contains('h1', headerText, {matchCase: false}).should('be.visible'); |
There was a problem hiding this comment.
I'm starting to wonder why these test assertions are about visibility. We don't do anything about it being visible, we are trying to assert they exist.
For the sidebar asserting visibility makes sense. They start in the DOM but not visible, and we are testing that if you click the button the resultant list becomes visible.
In most of these we really only need to test it exists in the DOM. We don't need to be worrying about scrolling into view
| @@ -30,10 +21,6 @@ declare namespace Cypress { | |||
| } | |||
| } | |||
|
|
|||
| Cypress.Commands.add('shouldBeVisible', {prevSubject: 'element'}, (subject) => { | |||
There was a problem hiding this comment.
So these commands are confusing me.
It looks like you've replaced all usage of shouldBeVisible with the native .should('be.visible'), but then replaced .should('have.attr', 'aria-expanded', 'true'); with shouldHaveAttribute in other places.
It looks like you pulled out the one command that was being used, and then started using another one for a different purpose.
I'm also not entirely convinced that
.shouldHaveAttribute('aria-expanded', 'true')
is much, if not any, improvement over the native
.should('have.attr', 'aria-expanded', 'true');
Maaaaaybe lets' just delete this file and commands entirely?
Description
This PR turns on the newest addition to our End to End (E2E) tests and introduces some optimizations to the Github Actions job (GHAJ) that runs them.
Summary of changes
strategyconfiguration for the Cypress GHAJ that allows our tests to run on 3 machines and to not fail fast if a single test case or suite fails (which causes the dashboard to hang),abortPrefetchRequeststo prevent prefetches on page load that slow down our testsheaderShouldBeVisibleto decrease the tests' coupling to the formatted contentensureScrollableconfig option togetInFixedContainerto cause tests to error out if the container it is called on is not in fact scrollable.Contributing to Twilio