-
Notifications
You must be signed in to change notification settings - Fork 579
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
Add e2e tests with Playwright #8521
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8521 +/- ##
==========================================
- Coverage 88.48% 88.48% -0.01%
==========================================
Files 276 276
Lines 27433 27433
==========================================
- Hits 24274 24273 -1
- Misses 3159 3160 +1 ☔ View full report in Codecov by Sentry. |
5003d9e
to
d66b485
Compare
A script like the following, which leverages import sys
from subprocess import PIPE, run
def get_changed_files():
cmd = "git diff --name-only -r HEAD^ main"
output = run(cmd, stdout=PIPE, stderr=PIPE, shell=True, universal_newlines=True)
files = output.stdout
return files
for new in get_changed_files().splitlines():
if not new.endswith(".spec.ts"):
continue
old = new.replace("e2e", "tests").replace(".spec.ts", "-test.js")
cmd = f"diff -u --ignore-space-change {old} {new} | delta"
output = run(cmd, stdout=PIPE, stderr=PIPE, shell=True, universal_newlines=True)
print(output.stdout)
sys.stdin.read(1) |
due to the size of the diff and my upcoming PTO I probably won't be able to review the PR this week, but I've put it on my todo list for next week :) |
Thanks! This PR definitely took some time to review, so I appreciate it. |
This comment was marked as outdated.
This comment was marked as outdated.
c036238
to
aedaeee
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.
LGTM!
Given the size of the diff, I won't pretend to have reviewed every single test in this PR, but the approach in general looks good to me.
I rebased the branch on top of main
to get rid of the merge conflict, and I fixed a CSS selector typo in the 404 test, which I noticed in the second commit.
One thing to consider: we might want to add an e2e/README.md
explaining the purpose of the folder and that this is still WIP. I guess the next step then would be CI integration?
Thanks for the review! I'll integrate this change into our CI in a follow-up PR. After this PR is merged, should we skip the overlapping Ember tests that have already been rewritten in Playwright? |
☔ The latest upstream changes (presumably 1a6567d) made this pull request unmergeable. Please resolve the merge conflicts. |
I think it's best to keep them running in parallel for now until we're confident about the new test setup |
This PR starts porting the current test suit that includes visits to Playwright. Additionally, the tests will be implemented in Typescript.
Steps to try locally:
Items for follow-up PRs:
For those who are familiar with QUnit but haven't used Playwright before, you may want to skim through intro, especially the writing-tests and running-and-debugging sections. Playwright offers some test and debug functionalities similar to QUnit:
--debug
flag: Similar to development mode.--ui
flag: Similar to the QUnit tests route.There is also an getting-started-vscode if you use it as your editor.