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

chore(tests): allow unknown args in e2e #20026

Merged
merged 9 commits into from Apr 8, 2024
Merged

Conversation

jhoward1994
Copy link
Contributor

@jhoward1994 jhoward1994 commented Apr 4, 2024

What does it do?

Specify debug as a yarg option

Why is it needed?

So we can run things like
yarn test:e2e --domains content-manager -- tests/e2e/tests/content-manager/cloning.spec.ts --debug
&
yarn test:e2e --domains content-manager -- tests/e2e/tests/content-manager/cloning.spec.ts

Copy link

vercel bot commented Apr 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
contributor-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 8, 2024 2:24pm

Copy link
Contributor

@innerdvations innerdvations left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

tests/scripts/run-e2e-tests.js Outdated Show resolved Hide resolved
@jhoward1994
Copy link
Contributor Author

hey @joshuaellis would you be able to take a look at this PR if you have as sec please? I can't see why these changes would cause the e2e tests to fail in this way, the same specs are passing locally for me but the CI has been failing in the same way each time :(

@innerdvations
Copy link
Contributor

innerdvations commented Apr 8, 2024

Just had a thought. Instead of adding the debug option, why don't we just change the package.json script to use -- --debug to pass the arg instead of manually adding it?

I'm not sure how that will interact with the user then adding their own -- (or not), but I would expect that worst case they just have to know NOT to use -- as expected for the test:e2e:debug script because it's already there.

@innerdvations
Copy link
Contributor

innerdvations commented Apr 8, 2024

Or even simpler solution for now, I'm also fine with flipping it back to true and updating the documentation to specify that if you want to run individual files, you must not do it after the --domains arg, ie, you have to do yarn test:e2e:debug --setup -c=1 sometest.spec.ts --domains=admin but that yarn test:e2e:debug --setup -c=1 --domains=admin sometest.spec.ts will fail.

(if that's true, I did a quick test and it seemed to work)

@jhoward1994
Copy link
Contributor Author

Do you mean to make this the command in package.json "test:e2e:debug": "node tests/scripts/run-e2e-tests.js -- --debug"

I feel like thats worse as you then can't do something like yarn test:e2e:debug --concurrency=1 --domains=content-manager

We could remove the :debug entry in package.json completely and document to add -- --debug to run in debug mode?

I'm not sure I understood your second comment, have I missed something?

Copy link
Contributor

@innerdvations innerdvations left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I like this way because it keeps our script clearly separate from playwright options. I think we can also revisit later and find a clean way to add back the test:e2e:debug if we want.

I'll take care of making similar changes on CLI tests in the next few weeks as I start to merge them.

@jhoward1994 jhoward1994 merged commit 16983e5 into v5/main Apr 8, 2024
13 of 15 checks passed
@jhoward1994 jhoward1994 deleted the fix/e2e-unknown-args branch April 8, 2024 14:18
@echoes-hq echoes-hq bot added the echoes/type: maintenance/testing For tests written to support improved automation QA label Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes/type: maintenance/testing For tests written to support improved automation QA pr: fix This PR is fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants