Enhance the forbidOnly mode message to guide the user towards the configuration option

[TomasHubelbauer]

Hi, I am putting this PR out as a feeler to see if there's interested in improving this error message, but the copy is by no means final and I am open to improvement suggestions.

My intention here is to:

• Explain what a "focused item" is - that we're talking about a test and it being focused is most likely down it using only

  Are there other types of "items"? Are there other ways to make them focused other than only?

• Explain why we're even in focused mode and how to control it

  The default scaffolded Playwright config file includes a forbidMode expression driven by whether CI=1 is set.
  I ran into this when trying to reproduce a CI issue locally so I had it set and unknowingly entered focus only mode.
  I wasn't aware this mode was a thing because I was using the default configuration from npm init and did not familiarize myself with all the options in it.

Is there a way to tell if we're in a TypeScript or JavaScript project in this function? I would use that to display the configuration file name with the right extension.

[pavelfeldman]

A couple of tests failed due to the change!

[TomasHubelbauer]

@pavelfeldman Yeah I will adjust them, but I am curious if you have a better suggestion for the message copy as well as whether there is a way to distinguish whether the project is in TypeScript or JavaScript first. Do you have any input there?

[dgozman]

a better suggestion for the message copy

I'd recommend something like

Error: item focused with ".only" is not allowed, either by 'forbidOnly' option in 'playwright.config.[jt]s' or --forbid-only flag: "${title}"

Or you can even make it more specific, see below.

whether there is a way to distinguish whether the project is in TypeScript or JavaScript first.

In the createRootSuite() function you can access config file via testRun.config.config.configFile and check whether --forbid-only was specified via testRun.config.configCLIOverrides.forbidOnly. Note that config file is optional.

[TomasHubelbauer]

@dgozman Is there any way to tell if the project was npm init'd with the language choice of JavaScript or TypeScript? That would help me print the right configuration file extension as a part of the message.

I have not updated the tests yet, will do so once I clone Playwright and figure out how to run the tests locally.

[TomasHubelbauer]

Something weird is going on with the tests.

I am following CONTRIBUTING.md and it misses a step where I had to do npx playwright install before npm test would run but even after, npm test wants to run 9k tests and I only want to run these tests:

• tests/playwright-test/reporter.spec.ts should report forbid-only error to reporter
• tests/playwright-test/runner.spec.ts it should not allow a focused test when forbid-only is used

When I change either of the tests to test.only, it takes no effect, npm test still wants to run 9k tests. I notice the command shown for running specific tests uses playwright test not npm test so I try running that with npx playwright test and get an error in the line of:

Error: Requiring @playwright/test second time

I return to npm test --grep=reporter but still, 9k tests are scheduled to run.

What am I doing wrong?

[dgozman]

@dgozman Is there any way to tell if the project was npm init'd with the language choice of JavaScript or TypeScript?

Nope. But you can check configFile as I suggested above, or event print the path to it.

[TomasHubelbauer]

Sorry, I misunderstood your suggestion at first! Now I get what you mean and how to get the right path.

[dgozman]

I am following CONTRIBUTING.md and it misses a step where I had to do npx playwright install before npm test

We should fix this! Send us a PR if you'd like 😄

When I change either of the tests to test.only, it takes no effect, npm test still wants to run 9k tests.

Right, that's test runner tests. You can run them with npm run ttest (note double t). This is probably missing from docs as well.

npm run ttest -- --grep "should report forbid-only error to reporter"

I try running that with npx playwright test

Yeah, that's not going to work. We are using complicated setup to run test runner tests, so npm run ttest is the way to go.

[TomasHubelbauer]

Send us a PR if you'd like 😄

I would! Will do.

npm run ttest -- --grep

Perfect, thanks, will also add to the PR!

One last thing: I will also add a note about needing to run npm run build before re-running the tests after making a change.

[TomasHubelbauer]

Alright, this should be good to go. The tests are passing locally and we even tests the config file existing in one and not existing in the other and the error message reflecting that now. Thanks for engaging on this PR and all the useful tips!

[TomasHubelbauer]

@microsoft-github-policy-service agree

[TomasHubelbauer]

Need to fix this:

runner.spec.ts:60:5 › it should not allow a focused test when forbid-only is used 
Error: expect(received).toContain(expected) // indexOf
Expected substring: "Error: … the Playwright configuration file: \"tests/focused-test.spec.js i-am-focused\""
Received string:    "Error: … the Playwright configuration file: \"tests\\focused-test.spec.js i-am-focused\"·

I need to use platform-specific path separators to make this pass on Windows. Coming up next.

[TomasHubelbauer]

I am getting random test failures that seem unrelated and IDK how to fix them 😞

[TomasHubelbauer]

There are again some failing tests on the CI but I do not think they relate to my changes.

[dgozman]

Thank you for the PR! Looks great, merging in.

[TomasHubelbauer]

Thank you!!!

[github-actions]

Primary test results: https://mspwblobreport.z1.web.core.windows.net/run-5084333456-e550df060ee6d8641df751d16b599a64876baf32-test_linux/index.html