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.

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

Only save test data for tests run with Replay browser #586

Merged
merged 3 commits into from
Jul 3, 2024
Merged

Only save test data for tests run with Replay browser #586

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
76e3988
Only save test data for tests run with Replay browser
bvaughn Jul 2, 2024
de90354
Add changelog
bvaughn Jul 2, 2024
d52c6a5
PR feedback
bvaughn Jul 3, 2024
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
5 changes: 5 additions & 0 deletions .changeset/late-cars-doubt.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@replayio/playwright": patch
---

Only save test data for tests run with Replay browser
73 changes: 44 additions & 29 deletions packages/playwright/src/reporter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,8 @@ export default class ReplayPlaywrightReporter implements Reporter {
string,
{ steps: FixtureStep[]; stacks: Record<string, StackFrame[]>; filenames: Set<string> }
> = {};
private _projects: Record<string, { executed: boolean; usingReplay: boolean }> = {};

private _executedProjects: Record<string, { usesReplayBrowser: boolean }> = {};

constructor(config: ReplayPlaywrightConfig) {
setUserAgent(`${packageName}/${packageVersion}`);
Expand Down Expand Up @@ -152,7 +153,7 @@ export default class ReplayPlaywrightReporter implements Reporter {
return this.fixtureData[id];
}

// Playwright alrady provides a unique test id:
// Playwright already provides a unique test id:
// https://github.com/microsoft/playwright/blob/6fb214de2378a9d874b46df6ea99d04da5765cba/packages/playwright/src/common/suiteUtils.ts#L56-L57
// this is different because it includes `repeatEachIndex` and `attempt`
// TODO(PRO-667): this could be simplified to `${test.testId}-${test.repeatEachIndex}-${test.attempt}`
Expand All @@ -175,25 +176,31 @@ export default class ReplayPlaywrightReporter implements Reporter {
};
}

onBegin({ version, projects }: FullConfig) {
const replayBrowserPath = getRuntimePath();
for (const project of projects) {
this._projects[project.name] = {
executed: false,
usingReplay: project.use.launchOptions?.executablePath === replayBrowserPath,
};
}
onBegin({ version }: FullConfig) {
this.reporter.setTestRunnerVersion(version);
this.reporter.onTestSuiteBegin();
}

private _registerExecutedProject(test: TestCase) {
const project = test.parent.project();
if (project) {
let projectMetadata = this._executedProjects[project.name];
if (!projectMetadata) {
projectMetadata = this._executedProjects[project.name] = {
usesReplayBrowser: project.use.launchOptions?.executablePath === getRuntimePath(),
};
}
return projectMetadata;
}

return null;
}

onTestBegin(test: TestCase, testResult: TestResult) {
const projectName = test.parent.project()?.name;
const projectMetadata = this._registerExecutedProject(test);

// it's important to handle the root project's name here and that's an empty string
if (typeof projectName === "string") {
this._projects[projectName].executed = true;
}
// Don't save metadata for non-Replay projects
if (projectMetadata?.usesReplayBrowser === false) return;
Copy link
Member

Choose a reason for hiding this comment

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

It's unclear to me when it can happen but, in theory (aka according to the types), the project is optional.... In such a scenario the tested value resolves to undefined here and based on that code will continue to process this. I wonder if that's intentional behavior or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was also unclear to me about when this could happen. (How would Playwright even know which browser to use if there was no project?)

I just did some local testing and realized that I can comment out the projects config block entirely and Playwright still works:

export default defineConfig({
  testDir: "./tests",
  fullyParallel: true,
  forbidOnly: !!process.env.CI,
  retries: 0,
  reporter: [
    ["dot"] as const,
    replayReporter({
      apiKey: process.env.REPLAY_API_KEY,
      upload: true,
    }),
  ],
  timeout: 10_000,
  use: {
    viewport: {
      width: 1280,
      height: 1024,
    },
  },
});

Adding some logging to the reporter I see that we still get a project back in this case though:

{
  grep: /.*/,
  grepInvert: null,
  outputDir: '/Users/bvaughn/Documents/git/replay/library/test-results',
  repeatEach: 1,
  retries: 0,
  metadata: {},
  name: '',
  testDir: '/Users/bvaughn/Documents/git/replay/library/tests',
  snapshotDir: '/Users/bvaughn/Documents/git/replay/library/tests',
  testIgnore: [],
  testMatch: '**/*.@(spec|test).?(c|m)[jt]s?(x)',
  timeout: 10000,
  use: { viewport: { width: 1280, height: 1024 } },
  dependencies: [],
  teardown: undefined,
  __projectId: ''
}

So the logic I have here would still hold. That being said, maybe it's safer to switch back to my original approach:

if (!projectMetadata?.usesReplayBrowser) return;


const testExecutionId = this._getTestExecutionId({
filePath: test.location.file,
Expand All @@ -202,6 +209,7 @@ export default class ReplayPlaywrightReporter implements Reporter {
attempt: testResult.retry + 1,
source: this.getSource(test),
});

this.reporter.onTestBegin(testExecutionId, getMetadataFilePath(testResult.workerIndex));
}

Expand Down Expand Up @@ -266,9 +274,15 @@ export default class ReplayPlaywrightReporter implements Reporter {

onTestEnd(test: TestCase, result: TestResult) {
const status = result.status;
// skipped tests won't have a reply so nothing to do here

// Skipped tests won't have a reply so nothing to do here
if (status === "skipped") return;

const projectMetadata = this._registerExecutedProject(test);

// Don't save metadata for non-Replay projects
if (projectMetadata?.usesReplayBrowser === false) return;

const testExecutionIdData = {
filePath: test.location.file,
projectName: test.parent.project()?.name,
Expand Down Expand Up @@ -351,25 +365,23 @@ export default class ReplayPlaywrightReporter implements Reporter {
try {
await this.reporter.onEnd();

const output: string[] = [];
const didUseReplayBrowser = Object.values(this._executedProjects).some(
({ usesReplayBrowser }) => usesReplayBrowser
);
const isReplayBrowserInstalled = existsSync(getRuntimePath());

const executedProjectWithReplay = !Object.keys(this._projects).some(projectName => {
const { executed, usingReplay } = this._projects[projectName];
return executed && usingReplay;
});
const output: string[] = [];

if (!executedProjectWithReplay) {
if (!didUseReplayBrowser) {
mixpanelAPI.trackEvent("playwright.warning.reporter-used-without-replay-project");
output.push(emphasize("None of the configured projects ran using Replay Chromium."));
}

if (!existsSync(getRuntimePath())) {
if (executedProjectWithReplay) {
if (!isReplayBrowserInstalled) {
if (didUseReplayBrowser) {
mixpanelAPI.trackEvent("playwright.warning.replay-browser-not-installed");
}
if (output.length) {
output.push("");
}

output.push(
`To record tests with Replay, you need to install the Replay browser: ${highlight(
"npx replayio install"
Expand All @@ -378,13 +390,16 @@ export default class ReplayPlaywrightReporter implements Reporter {
}

if (output.length) {
output.push("");
output.push(
`Learn more at ${link(
"https://docs.replay.io/reference/test-runners/playwright/overview"
)}`
);
output.forEach(line => {

output.forEach((line, index) => {
if (index > 0) {
console.log("[replay.io]:");
}
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
console.warn(`[replay.io]: ${line}`);
});
}
Expand Down
26 changes: 16 additions & 10 deletions packages/test-utils/src/reporter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,12 +446,7 @@ export default class ReplayReporter<
return;
}

if (this._testRunShardIdPromise) {
return;
}

this._testRunShardIdPromise = this._startTestRunShard();
this._pendingWork.push(this._testRunShardIdPromise);
// Don't event record test metadata yet unless/until a test is run with the Replay browser (see onTestBegin)
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
}

private async _startTestRunShard(): Promise<TestRunPendingWork> {
Expand Down Expand Up @@ -673,6 +668,15 @@ export default class ReplayReporter<
onTestBegin(testExecutionId?: string, metadataFilePath = getMetadataFilePath("REPLAY_TEST", 0)) {
logger.info("OnTestBegin:Started", { testExecutionId });

if (this._apiKey) {
if (!this._testRunShardIdPromise) {
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
// This method won't be called until a test is run with the Replay browser
// We shouldn't save any test metadata until that happens
this._testRunShardIdPromise = this._startTestRunShard();
this._pendingWork.push(this._testRunShardIdPromise);
}
}

this._errors = [];
const metadata = {
...(this._baseMetadata || {}),
Expand Down Expand Up @@ -1230,14 +1234,14 @@ export default class ReplayReporter<
numUploaded = uploaded.length;

if (uploaded.length > 0) {
output.push(`\n🚀 Successfully uploaded ${uploads.length} recordings:\n`);
output.push(`\n🚀 Successfully uploaded ${uploads.length} recordings:`);
const sortedUploads = sortRecordingsByResult(uploads);
sortedUploads.forEach(r => {
output.push(
` ${getTestResultEmoji(r)} ${(r.metadata.title as string | undefined) || "Unknown"}`
`\n ${getTestResultEmoji(r)} ${(r.metadata.title as string | undefined) || "Unknown"}`
);
output.push(
` ${process.env.REPLAY_VIEW_HOST || "https://app.replay.io"}/recording/${r.id}\n`
` ${process.env.REPLAY_VIEW_HOST || "https://app.replay.io"}/recording/${r.id}`
Comment on lines -1233 to +1242
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was leaving a goofy trailing line too, after the last test.

);
});
}
Expand All @@ -1256,7 +1260,9 @@ export default class ReplayReporter<
numUploaded,
});

log(output.join("\n"));
if (output.length > 0) {
log(output.join("\n"));
}
bvaughn marked this conversation as resolved.
Show resolved Hide resolved

return results;
}
Expand Down