Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions packages/cli/src/commands/debug/parse-project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,9 @@ export default class ParseProjectCommand extends Command {
checklyConfigConstructs,
playwrightConfigPath: checklyConfig.checks?.playwrightConfigPath,
include: includeFlag.length ? includeFlag : checklyConfig.checks?.include,
includeFlagProvided: includeFlag.length > 0,
playwrightChecks: checklyConfig.checks?.playwrightChecks,
currentCommand: emulatePwTest ? 'pw-test' : undefined,
loadPlaywrightChecksOnly: emulatePwTest,
warnOnWebServerConfig: emulatePwTest && !(includeFlag.length > 0),
})

const diagnostics = new Diagnostics()
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/commands/pw-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,9 +195,9 @@ export default class PwTestCommand extends AuthCommand {
checklyConfigConstructs,
playwrightConfigPath,
include: includeFlag.length ? includeFlag : checklyConfig.checks?.include,
includeFlagProvided: includeFlag.length > 0,
playwrightChecks: [playwrightCheck],
currentCommand: 'pw-test',
loadPlaywrightChecksOnly: true,
warnOnWebServerConfig: !(includeFlag.length > 0),
checkFilter: check => {
// Skip non Playwright checks
if (!(check instanceof PlaywrightCheck)) {
Expand Down
Copy link
Member Author

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
3 changes: 1 addition & 2 deletions packages/cli/src/constructs/playwright-check.ts
Original file line number Diff line number Diff line change
Expand Up @@ -283,8 +283,7 @@ export class PlaywrightCheck extends RuntimeCheck {
}

protected async validateWebServerConfig (diagnostics: Diagnostics): Promise<void> {
// Only show webServer warning for pw-test command when --include is not provided
if (Session.currentCommand !== 'pw-test' || Session.includeFlagProvided) {
if (!Session.warnOnWebServerConfig) {
return
}

Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/constructs/project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -248,8 +248,7 @@ export class Session {
static parsers = new Map<string, Parser>()
static constructExports: ConstructExport[] = []
static ignoreDirectoriesMatch: string[] = []
static currentCommand?: 'pw-test' | 'test' | 'deploy'
static includeFlagProvided?: boolean
static warnOnWebServerConfig?: boolean
static packageManager: PackageManager = npmPackageManager
static workspace: Result<Workspace, Error> = Err(new Error(`Workspace support not initialized`))

Expand All @@ -272,6 +271,7 @@ export class Session {
this.parsers = new Map<string, Parser>()
this.constructExports = []
this.ignoreDirectoriesMatch = []
this.warnOnWebServerConfig = false
this.packageManager = npmPackageManager
this.workspace = Err(new Error(`Workspace support not initialized`))
this.resetSharedFiles()
Expand Down
30 changes: 17 additions & 13 deletions packages/cli/src/services/project-parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@ type ProjectParseOpts = {
checklyConfigConstructs?: Construct[]
playwrightConfigPath?: string
include?: string | string[]
includeFlagProvided?: boolean
playwrightChecks?: PlaywrightSlimmedProp[]
currentCommand?: 'pw-test' | 'test' | 'deploy'
loadPlaywrightChecksOnly?: boolean
warnOnWebServerConfig?: boolean
enableWorkspaces?: boolean
}

Expand Down Expand Up @@ -144,9 +144,9 @@ export async function parseProject (opts: ProjectParseOpts): Promise<Project> {
checklyConfigConstructs,
playwrightConfigPath,
include,
includeFlagProvided,
playwrightChecks,
currentCommand,
loadPlaywrightChecksOnly,
warnOnWebServerConfig,
enableWorkspaces = true,
} = opts

Expand All @@ -166,7 +166,11 @@ export async function parseProject (opts: ProjectParseOpts): Promise<Project> {
ignoreWorkspaces: !enableWorkspaces,
})

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

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 @@ -179,20 +183,20 @@ export async function parseProject (opts: ProjectParseOpts): Promise<Project> {
Session.defaultRuntimeId = defaultRuntimeId
Session.verifyRuntimeDependencies = verifyRuntimeDependencies ?? true
Session.ignoreDirectoriesMatch = ignoreDirectoriesMatch
Session.currentCommand = currentCommand
Session.includeFlagProvided = includeFlagProvided
Session.warnOnWebServerConfig = warnOnWebServerConfig
Session.packageManager = packageManager
Session.workspace = workspace

// 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)

// Load sequentially because otherwise Session.checkFileAbsolutePath and
// Session.checkFilePath are going to be subject to race conditions.
await loadAllBrowserChecks(directory, browserCheckMatch, ignoreDirectories, project)
await loadAllMultiStepChecks(directory, multiStepCheckMatch, ignoreDirectories, project)
if (!loadPlaywrightChecksOnly) {
await loadAllCheckFiles(directory, checkMatch, ignoreDirectories)
// Load sequentially because otherwise Session.checkFileAbsolutePath and
// Session.checkFilePath are going to be subject to race conditions.
await loadAllBrowserChecks(directory, browserCheckMatch, ignoreDirectories, project)
await loadAllMultiStepChecks(directory, multiStepCheckMatch, ignoreDirectories, project)
}
await loadPlaywrightChecks(directory, playwrightChecks, playwrightConfigPath, include)

// private-location must be processed after all checks and groups are loaded.
Expand Down
Loading