Allow for extra Playwright CLI options when running e2e tests #1768
Conversation
jdeanwallace
changed the title
jdeanwallace
changed the title
Automated comment from CodeApprove ➜⏳ @jotaen4tinypilot please review this Pull Request |
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
⏳ Approval Pending (1 unresolved comments)
Approval will be granted automatically when all comments are resolved
In: dev-scripts/run-e2e-tests:
> Line 40
# If the first positional argument starts with 'http', strip it to export it as
Just bouncing this as a thought here: I’m a bit on the fence with providing two optional positional arguments, as that is ambiguous to parse. Without reading this code comment, it might not be 100% clear from the outside how this script disambiguates the two arguments.
In my mind, the ideal and most “conventional” way for the script API would be:
- Make
E2E_BASE_URLa named flag, like--baseUrl http://... - Separate the script flags from the Playwright flags via a
--separator
That way, you could do:
./run-e2e-tests --baseUrl http://localhost:123 -- --grep 'foo'
That being said, I think we’ve never used the -- separator in our scripts, so I don’t know how much work it would be, or whether it’d be worth to invest that for an internal dev script.
So depending on what your thoughts are, to me it would be fine to leave this as proposed, though I think we could then address the flag parsing behaviour more explicitly in the help output.
👀 @jdeanwallace it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: dev-scripts/run-e2e-tests:
Oh yeah, that makes sense. I was intrigued by the -- idea, so did implement it here.
Can you check once again if all is in order?
👀 @jotaen4tinypilot it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: dev-scripts/run-e2e-tests:
Neat!
In: dev-scripts/run-e2e-tests:
> Line 88
npx playwright test "${PLAYWRIGHT_ARGS[@]+"${PLAYWRIGHT_ARGS[@]}"}"
This is some wacky bash!
Can we add a comment explaining what it's doing? I feel like nobody will understand it except for like the top 5% of bash developers.
👀 @jdeanwallace, @jotaen4tinypilot it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
⏳ Approval Pending (3 unresolved comments)
Approval will be granted automatically when all comments are resolved
In: dev-scripts/run-e2e-tests:
🎉
In: dev-scripts/run-e2e-tests:
> Line 19
-- Optional. Indicate the end of this script's CLI
I’m wondering whether we could consolidate -- and PLAYWRIGHT_ARGS into a single help item. My reasoning would be that they are not independent from each other, but it’s either both or none (i.e., you specify -- to enter Playwright-args land).
This should in theory be clear from the Usage string definition, which we only reference here, but I think there might still be potential room for misunderstanding.
In: dev-scripts/run-e2e-tests:
> Line 48
shift
What would you think about initializing $PLAYWRIGHT_ARGS as empty array (PLAYWRIGHT_ARGS=()) above the while, and then doing the ("$@") assignment here? I’m thinking that this could make the code more cohesive, and thus slightly easier to follow – like:
--)
# Stop parsing command-line arguments, and capture all remaining
# args to pass them through to Playwright.
shift
PLAYWRIGHT_ARGS=("$@")
breakIn: dev-scripts/run-e2e-tests:
> Line 88
npx playwright test "${PLAYWRIGHT_ARGS[@]+"${PLAYWRIGHT_ARGS[@]}"}"
Would it be safe to always pass on $PLAYWRIGHT_ARGS to npx ..., even if it’s empty? Or is there a scenario where Playwright might complain?
👀 @jdeanwallace, @mtlynch it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: dev-scripts/run-e2e-tests:
Done.
In: dev-scripts/run-e2e-tests:
> Line 49
shift
Done.
In: dev-scripts/run-e2e-tests:
> Line 93
npx playwright test "${PLAYWRIGHT_ARGS[@]+"${PLAYWRIGHT_ARGS[@]}"}"
Can we add a comment explaining what it's doing?
Done.
Would it be safe to always pass on $PLAYWRIGHT_ARGS to npx ..., even if it’s empty? Or is there a scenario where Playwright might complain?
Yes, if $PLAYWRIGHT_ARGSis empty, nothing is passed to npx.
👀 @mtlynch it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
Approved: I have approved this change on CodeApprove and all of my comments have been resolved.
Resolves #1696
This PR allows for extra Playwright CLI options to passed to Playwright when running e2e tests.
For example, this means you can guide Playwright on which tests to run like so:
./dev-scripts/run-e2e-tests -- --grep 'shows privacy policy'