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

Switch TPv2 E2E tests to Cypress #7894

Merged
merged 39 commits into from
Jan 22, 2024
Merged

Conversation

ocket8888
Copy link
Contributor

This PR switches the TPv2 E2E testing suite from NightwatchJS to Cypress.

"Why?", you may ask. Because it took me all of two days to replicate all of our test suite in Cypress - that's how easy it is to use. For comparison: I have never even once been able to get the Nightwatch tests to run on my machine. With Cypress, the typings are provided by the library itself, testing on Chrome and Firefox took exactly zero time and effort to figure out, and the tests actually just do the things I want them to do without me having to spend countless hours needling them with the correct set of awaits and chained commands. Which is why most all of our tests do little more than check for the presence of some DOM elements. Maybe with Cypress, we could fix that.

tl;dr: Nightwatch is extremely hard to use and Cypress is extremely easy.


Which Traffic Control components are affected by this PR?

  • Traffic Portal (experimental v2)

What is the best way to verify this PR?

Run the end-to-end tests. Which doesn't take very long and can be depended upon to actually work.

PR submission checklist

  • This PR has tests
  • This PR has documentation
  • This PR doesn't need a CHANGELOG.md entry
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY

@ocket8888 ocket8888 added low impact affects only a small portion of a CDN, and cannot itself break one tech debt rework due to choosing easy/limited solution experimental a feature/component not directly supported by ATC Traffic Portal v2 Related to the experimental Traffic Portal version 2 labels Jan 6, 2024
experimental/traffic-portal/cypress.config.ts Dismissed Show dismissed Hide dismissed
Copy link

codecov bot commented Jan 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (adea91d) 28.84% compared to head (f232748) 74.35%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@              Coverage Diff              @@
##             master    #7894       +/-   ##
=============================================
+ Coverage     28.84%   74.35%   +45.50%     
=============================================
  Files           602      117      -485     
  Lines         77445     5374    -72071     
  Branches         90      880      +790     
=============================================
- Hits          22342     3996    -18346     
+ Misses        53012     1310    -51702     
+ Partials       2091       68     -2023     
Flag Coverage Δ
golib_unit ?
grove_unit ?
t3c_generate_unit ?
t3c_unit ?
traffic_monitor_unit ?
traffic_ops_integration ?
traffic_ops_unit ?
traffic_portal_v2 74.35% <ø> (?)
traffic_router_unit ?
traffic_stats_unit ?
unit_tests 74.35% <ø> (+48.68%) ⬆️
v3 ?
v4 ?
v5 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shamrickus shamrickus merged commit d8e78de into apache:master Jan 22, 2024
9 checks passed
@ocket8888 ocket8888 deleted the tpv2/cypress branch January 22, 2024 02:29
@zrhoffman
Copy link
Member

This is making the TPv2 RPM fail to build

From the PR of #7922:

npm ERR! Cypress cannot write to the cache directory due to file permissions

@ocket8888
Copy link
Contributor Author

that's interesting. It shouldn't be attempting to compile or run the e2e tests to build the RPM.

zrhoffman added a commit that referenced this pull request Jan 23, 2024
@zrhoffman
Copy link
Member

Reverted due to breaking tests in 4b6af89, please re-PR with tests fixed

@ocket8888
Copy link
Contributor Author

Reverting something experimental because it affects the ability to build distributables for something that isn't distributed (because it's experimental) seems a bit extreme.

@zrhoffman
Copy link
Member

If we don't want it to affect the default builds, then

  • It should not be built when someone just runs ./pkg without specifying optional builds with -o, and
  • It should not be included in the CDN-in-a-Box CI GHA workflow.

But because it is included in ./pkg by default and the CDN-in-a-Box CI GHA workflow, it broke tests for completely unrelated components, and that's why it was reverted.

@ocket8888
Copy link
Contributor Author

  • "It should not be built when someone just runs ./pkg without specifying optional builds with -o, and"

That should definitely never have been true. That's a bug in pkg, far as I'm concerned.

  • "It should not be included in the CDN-in-a-Box CI GHA workflow."

Also should never have been true, but additionally: why is a test that depends on and explicitly uses something not run when that thing changes? Two bugs in the GHAs.

@ocket8888 ocket8888 restored the tpv2/cypress branch January 23, 2024 19:54
@zrhoffman
Copy link
Member

AFAIAC, the bug that lead to the revert is that TPv2 is included in the CDN-in-a-Box CI GHA workflow but its paths don't trigger it. If the CDN-in-a-Box CI GHA had failed (due to the breaking changes in #7894), it would not have gotten merged and would not need to be reverted.

@ocket8888 ocket8888 mentioned this pull request Jan 23, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental a feature/component not directly supported by ATC low impact affects only a small portion of a CDN, and cannot itself break one tech debt rework due to choosing easy/limited solution Traffic Portal v2 Related to the experimental Traffic Portal version 2
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants