-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
tests(devtools): remove webtests, sync e2e tests, use release mode #14163
Conversation
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.
Some version of https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3721466 will fix the problem, and we already made these tests optional for merging PRs. Is there an urgent reason to remove these tests?
We're committed to dropping these tests, have already started porting a couple, and won't do another CDT release before that process finishes. No need to continue playing the whack-a-mole game anymore. They'll still be in the CDT frontend for now, in case they catch something important. |
I'm not so sure, it seems like our web tests are non-fatal in DT frontend. They didn't catch the issue in https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3721466 before it happened. I guess that makes us the only place the tests "matter"? Since we're not doing another release until it's done I'm fine with merging this, but it seems like https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3721466 could buy us a few more weeks of use instead of creating a blind spot in our tests for a few weeks. |
We can remove these tests now, because we're starting to migrate to e2e tests (#14110). Pretty sure the devtools smoke tests will cover things just as well in the interim, but we can consider 10.0 blocking on the e2e migration.