From e550df060ee6d8641df751d16b599a64876baf32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20H=C3=BCbelbauer?= Date: Thu, 25 May 2023 22:32:49 +0200 Subject: [PATCH] Enhance the forbidOnly mode message to guide the user towards the configuration option (#23146) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --------- Signed-off-by: Tomáš Hübelbauer --- CONTRIBUTING.md | 10 ++++++++++ packages/playwright-test/src/runner/loadUtils.ts | 12 ++++++++---- tests/playwright-test/reporter.spec.ts | 2 +- tests/playwright-test/runner.spec.ts | 9 +++++++-- 4 files changed, 26 insertions(+), 7 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 69a786bbe5..fcbc095f98 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -150,14 +150,24 @@ These are integration tests, making sure public API methods and events work as e - To run all tests: ```bash +npx playwright install npm run test ``` +Be sure to run `npm run build` or let `npm run watch` run before you re-run the +tests after making your changes to check them. + - To run all tests in Chromium ```bash npm run ctest # also `ftest` for firefox and `wtest` for WebKit ``` +- To run the Playwright test runner tests +```bash +npm run ttest +npm run ttest -- --grep "specific test" +``` + - To run a specific test, substitute `it` with `it.only`, or use the `--grep 'My test'` CLI parameter: ```js diff --git a/packages/playwright-test/src/runner/loadUtils.ts b/packages/playwright-test/src/runner/loadUtils.ts index 55fd9a33b3..5b99829905 100644 --- a/packages/playwright-test/src/runner/loadUtils.ts +++ b/packages/playwright-test/src/runner/loadUtils.ts @@ -148,8 +148,10 @@ export async function createRootSuite(testRun: TestRun, errors: TestError[], sho // Complain about only. if (config.config.forbidOnly) { const onlyTestsAndSuites = rootSuite._getOnlyItems(); - if (onlyTestsAndSuites.length > 0) - errors.push(...createForbidOnlyErrors(onlyTestsAndSuites)); + if (onlyTestsAndSuites.length > 0) { + const configFilePath = config.config.configFile ? path.relative(config.config.rootDir, config.config.configFile) : undefined; + errors.push(...createForbidOnlyErrors(onlyTestsAndSuites, config.configCLIOverrides.forbidOnly, configFilePath)); + } } // Filter only for top-level projects. @@ -220,13 +222,15 @@ async function createProjectSuite(fileSuites: Suite[], project: FullProjectInter return projectSuite; } -function createForbidOnlyErrors(onlyTestsAndSuites: (TestCase | Suite)[]): TestError[] { +function createForbidOnlyErrors(onlyTestsAndSuites: (TestCase | Suite)[], forbidOnlyCLIFlag: boolean | undefined, configFilePath: string | undefined): TestError[] { const errors: TestError[] = []; for (const testOrSuite of onlyTestsAndSuites) { // Skip root and file. const title = testOrSuite.titlePath().slice(2).join(' '); + const configFilePathName = configFilePath ? `'${configFilePath}'` : 'the Playwright configuration file'; + const forbidOnlySource = forbidOnlyCLIFlag ? `'--forbid-only' CLI flag` : `'forbidOnly' option in ${configFilePathName}`; const error: TestError = { - message: `Error: focused item found in the --forbid-only mode: "${title}"`, + message: `Error: item focused with '.only' is not allowed due to the ${forbidOnlySource}: "${title}"`, location: testOrSuite.location!, }; errors.push(error); diff --git a/tests/playwright-test/reporter.spec.ts b/tests/playwright-test/reporter.spec.ts index aecc6b9838..f15cd9224d 100644 --- a/tests/playwright-test/reporter.spec.ts +++ b/tests/playwright-test/reporter.spec.ts @@ -547,7 +547,7 @@ test('should report forbid-only error to reporter', async ({ runInlineTest }) => }, { 'reporter': '', 'forbid-only': true }); expect(result.exitCode).toBe(1); - expect(result.output).toContain(`%%got error: Error: focused item found in the --forbid-only mode`); + expect(result.output).toContain(`%%got error: Error: item focused with '.only' is not allowed due to the '--forbid-only' CLI flag: \"a.test.ts pass\"`); }); test('should report no-tests error to reporter', async ({ runInlineTest }) => { diff --git a/tests/playwright-test/runner.spec.ts b/tests/playwright-test/runner.spec.ts index 8fcec7630b..8d48385a0c 100644 --- a/tests/playwright-test/runner.spec.ts +++ b/tests/playwright-test/runner.spec.ts @@ -59,13 +59,18 @@ test('it should not allow multiple tests with the same name in multiple files', test('it should not allow a focused test when forbid-only is used', async ({ runInlineTest }) => { const result = await runInlineTest({ + 'playwright.config.ts': ` + module.exports = { + forbidOnly: true, + }; + `, 'tests/focused-test.spec.js': ` import { test, expect } from '@playwright/test'; test.only('i-am-focused', async () => {}); ` - }, { 'forbid-only': true }); + }); expect(result.exitCode).toBe(1); - expect(result.output).toContain('Error: focused item found in the --forbid-only mode'); + expect(result.output).toContain(`Error: item focused with '.only' is not allowed due to the 'forbidOnly' option in 'playwright.config.ts': \"tests${path.sep}focused-test.spec.js i-am-focused\"`); expect(result.output).toContain(`test.only('i-am-focused'`); expect(result.output).toContain(`tests${path.sep}focused-test.spec.js:3`); });