Skip to content

fix: skip PlaywrightCheck constructs during pw-test [RED-155] #1223

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.

Sign up for GitHub

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

Open
miliberlin wants to merge 9 commits into
base: main
Choose a base branch
from
+25 −21
Open

fix: skip PlaywrightCheck constructs during pw-test [RED-155] #1223

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ac421b0
fix: skip PlaywrightCheck constructs and check files during pw-test […
miliberlin Feb 16, 2026
78ffee0
fix: update webServer test fixture to use playwrightConfigPath for ad…
miliberlin Feb 16, 2026
2aa2868
refactor: replace currentCommand with explicit behavior flags [RED-155]
miliberlin Feb 17, 2026
1bbf380
refactor: rename skipWebServerValidation to warnOnWebServerConfig
miliberlin Feb 17, 2026
1c62dfa
refactor: rename flag to loadPlaywrightChecksOnly and skip all check …
miliberlin Feb 17, 2026
01e3ac2
fix: reset warnOnWebServerConfig in Session.reset()
miliberlin Feb 18, 2026
d3d0843
Merge branch 'main' into michelle/red-155-pw-test-should-ignore-playw…
miliberlin Feb 18, 2026
300595e
Merge branch 'main' into michelle/red-155-pw-test-should-ignore-playw…
miliberlin Feb 19, 2026
e919916
Merge branch 'main' into michelle/red-155-pw-test-should-ignore-playw…
miliberlin Feb 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...onstructs/__tests__/fixtures/playwright-check/test-cases/test-webServer/checkly.config.ts
View file
Open in desktop
Copy link
Member Author

@miliberlin miliberlin Feb 17, 2026

Choose a reason for hiding this comment

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

Summary of why we need to tweak the config for this test below. I think this is correct since this is how a user would set up the project, but please double-check.

Before our fix, the webServer test worked like this:

  1. parseProject() runs with --emulate-pw-test
  2. loadAllCheckFiles() picks up test.check.ts, which calls new PlaywrightCheck(...) with playwrightConfigPath: './playwright.config.ts'
  3. The PlaywrightCheck constructor runs validateWebServerConfig(), sees webServer in the playwright config, and emits the warning

Our fix adds if (currentCommand !== 'pw-test') around loadAllCheckFiles(), so step 2 never happens — test.check.ts is never loaded, no PlaywrightCheck is created, no warning fires.

But there's a second code path that creates PlaywrightCheck instances: loadPlaywrightChecks(). This function still runs for pw-test. It creates an "ad-hoc" PlaywrightCheck when playwrightConfigPath is set in the checkly config — this is the same path that real users hit when they have checks: { playwrightConfigPath: './playwright.config.ts' } in their checkly.config.ts.

By adding playwrightConfigPath to the fixture's config, the ad-hoc check gets created via loadPlaywrightChecks() instead of via loadAllCheckFiles(). The webServer validation fires on this ad-hoc check, and the test passes.

This also makes the test more realistic — in practice, pw-test users always have playwrightConfigPath in their config (that's how pw-test knows which playwright config to use). The old fixture relied on a .check.ts file to create the PlaywrightCheck, which is exactly the scenario our fix is designed to skip.

Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const config = defineConfig({
logicalId: 'playwright-check-fixture',
checks: {
checkMatch: '**/*.check.ts',
playwrightConfigPath: './playwright.config.ts',
},
})

Expand Down
10 changes: 8 additions & 2 deletions packages/cli/src/services/project-parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,11 @@ export async function parseProject (opts: ProjectParseOpts): Promise<Project> {
ignoreWorkspaces: !enableWorkspaces,
})

checklyConfigConstructs?.forEach(
const filteredConstructs = currentCommand === 'pw-test'
? checklyConfigConstructs?.filter(c => !(c instanceof PlaywrightCheck))
: checklyConfigConstructs
Comment on lines +169 to +171
Copy link
Member

@sorccu sorccu Feb 20, 2026

Choose a reason for hiding this comment

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

Not completely sure if this is the correct thing to do. Even when you define a PlaywrightCheck in the config, it could potentially refer to a group construct or alert channel constructs. It could, for example, lead to group defaults not being applied. It might also be completely fine. Could you investigate a bit and see if it's OK or not?


filteredConstructs?.forEach(
construct => project.addResource(construct.type, construct.logicalId, construct),
)
Session.project = project
Expand All @@ -186,7 +190,9 @@ export async function parseProject (opts: ProjectParseOpts): Promise<Project> {
// TODO: Do we really need all of the ** globs, or could we just put node_modules?
const ignoreDirectories = ['**/node_modules/**', '**/.git/**', ...ignoreDirectoriesMatch]

await loadAllCheckFiles(directory, checkMatch, ignoreDirectories)
if (currentCommand !== 'pw-test') {
Copy link
Member

@sorccu sorccu Feb 17, 2026

Choose a reason for hiding this comment

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

I don't like this currentCommand thing, especially now that it's being used more. It feels like we're just adding a whole bunch of special cases based on the identity of the caller, which is IMO unpredictable and just ass in general. Instead of the caller identifying itself, it should request specific behavior via a relevant flag.

Maybe we can keep this as is if you need this out soon but I do NOT want to see this become a standard.

Copy link
Member Author

@miliberlin miliberlin Feb 17, 2026

Choose a reason for hiding this comment

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

@sorccu I gave replacing currentCommand a shot. Wdyt? The name of the flags can probably be improved, but I think as long as currentCommand exists it will be picked up again and again, better to get it out of the codebase if you don't like it.

Copy link
Member

@sorccu sorccu Feb 18, 2026

Choose a reason for hiding this comment

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

I like your change, let me go over the logic in a bit.

await loadAllCheckFiles(directory, checkMatch, ignoreDirectories)
}

// Load sequentially because otherwise Session.checkFileAbsolutePath and
// Session.checkFilePath are going to be subject to race conditions.
Expand Down
Loading