fix: Fix missing it.skip function in Angular tests
#23829
Conversation
|
Thanks for taking the time to open a PR!
|
mike-plummer
requested review from
jordanpowell88,
ZachJW34,
astone123 and
lmiller1990
| systemTests.it(`v${majorVersion} Zone.JS does not break Mocha`, { | ||
| project: `angular-${majorVersion}`, | ||
| spec: 'src/app/zonejs-mocha-skip.cy.ts,src/app/zonejs-mocha-only.cy.ts', | ||
| testingType: 'component', |
There was a problem hiding this comment.
I actually think this is the wrong place for this test. Also, we are not actually verifying that it.skip actually does what's it supposed to, only that it is available.
We could move this to a Cypress in Cypress test in app/cypress/e2e, and we could then assert against the command log that the correct number of tests are executed and/or skipped? That would verify not only that it works, but skip is the correct one (since we will see it visually marked as skipped in the Commanad Log, as well as the number of executed tests).
There was a problem hiding this comment.
You would add it here: https://github.com/cypress-io/cypress/blob/develop/packages/app/cypress/e2e/cypress-in-cypress-component.cy.ts
If you haven't seen the Cypress in Cypress yet it's really cool - you can still use this project (in system tests) but you can now assert things are actually happening in the UI, too.
What do you think?
There was a problem hiding this comment.
@lmiller1990 It looks like cypress-in-cypress is configured to use React for CT, so were you thinking we would stand up a new cypress-in-cypress permutation using Angular?
There was a problem hiding this comment.
We don't need to use cypress-in-cypress to test against the command log, we have an existing pattern where we point Cypress at the command log inside of system-tests here. I could go either way, but I'm a fan of testing as much as possible in system-tests and only using cy-in-cy if absolutely required since the system-tests will always be less costly.
There was a problem hiding this comment.
Took the approach Zach proposed to write some tests to validate the behavior of skip and only. Definitely not as elegant as cypress-in-cypress, but very performant and seems to provide the validation that was requested
There was a problem hiding this comment.
Oh I hadn't seen that @ZachJW34, neat.
The reason I suggested cypress-in-cypress is we have these really cool helpers:
cypress/packages/app/cypress/e2e/support/execute-spec.ts
Lines 14 to 18 in 6ee305b
You can do
cy.waitForSpecToFinish({
passCount: 4
failCount: 0
pendingCount: 1
})Which is exactly what we want here - it's way more readable, at least to me. Maybe we should find a way to share more of these commands between cypress-in-cypress and system-tests in the future.
Anyway, as long as we've got something preventing this regression, I'm happy.
|
Test summaryRun details
View run in Cypress Dashboard ➡️ 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 |
||||||||||||||||||||||||||||||||||
User facing changelog
it.skipwas not available in Angular component testsAdditional details
zone.js/testinglibrary patches Mocha to be "zone-aware", but the patch does not includeit.skip. This is written up in that repo under this issue, but it's not seeing any movement. Since this is a relatively trivial fix on our end and has annoying impacts on our users it seemed better to patch on our side pending any potential fix from that library.While debugging this issue I wrote up tests that validate
skipandonlyfor all major Mocha syntax blocks - I've added them to this PR even thoughonlyisn't really related since they seemed like useful checks to have in case Zone breaks these in the futureSteps to test
System Tests should validate
Manual validation would involve creating an NG project, installing Cypress and patching in updated
angularpackage and validating that a test withit.skipdoes not fail with a parsing errorHow has the user experience changed?
Users should be able to use
it.skipin Angular testsPR Tasks
cypress-documentation?type definitions?