Improve the warning about Replay-less executions #585

Andarist · 2024-07-02T12:26:26Z

No description provided.

changeset-bot · 2024-07-02T12:26:29Z

🦋 Changeset detected

Latest commit: 0e55651

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@replayio/playwright	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Andarist · 2024-07-02T12:27:40Z

packages/playwright/src/reporter.ts

@@ -164,14 +167,24 @@ class ReplayPlaywrightReporter implements Reporter {

  onBegin({ version, projects }: FullConfig) {
    const replayBrowserPath = getRuntimePath();
-    this._foundReplayBrowser = !!projects.find(


@bvaughn noticed that projects contain all configured projects - not only those that are executed in the test run.

So now, I'm tracking which projects are configured with Replay and then which are actually executed. That allows us to create a precise warning at the end of the run

replay-io · 2024-07-02T12:29:54Z

Status	Complete ↗︎
Commit	`7e2ac9d`
Results	❌ 2 Failed clicks a disappearing button should fail on this test ✅ 42 Passed adds items adds new items using a custom command adds todos following the fixture adds todos following the fixture adds todos following the fixture adds todos following the fixture calls inform complete all checkbox should update state when items are completed / cleared gets a number only gets a number only gets a number should allow me to add todo items should allow me to clear the complete state of all items should allow me to display active items should allow me to display all items should allow me to display completed items should allow me to edit an item should allow me to mark all items as completed should allow me to mark items as complete should allow me to un-mark items as complete should append new items to the bottom of the list should be hidden when there are no items that are completed should cancel edits on escape should clear text input field when an item is added should display the correct text should display the current number of todo items should focus on the todo input field should hide #main and #footer should hide other controls when editing should highlight the currently applied filter should intercept postman should invoke some commands that have exceptional option handling should log should persist its data should remove completed items when clicked should remove the item if an empty text string was entered should respect the back button should save edits on blur should show #main and #footer when items added should trim entered text should trim text input yields a number

bvaughn · 2024-07-02T12:46:28Z

packages/playwright/src/reporter.ts

+            : `${projectsWithoutReplay.join(", ")} project${
+                projectsWithoutReplay.length > 1 ? "s" : ""
+              }`;
+


I'm not sure this produces a meaningfully better warning message to be worth it. I think just saying "Your tests ran without Replay Chromium" would be sufficient.

What would this actually look like in practice? My experience is pretty limited here, but the way I've typically seen projects named were like "firefox" or "chrome" So in that case this would print a warning like:

chrome project ran without Replay Chromium. If this wasn't intentional, please recheck your configuration.

or

firefox, chrome projects ran without Replay Chromium. If this wasn't intentional, please recheck your configuration.

Both of those seem kind of awkward to me, particularly if the project has a 1:1 relationship with a browser. I guess in most cases it would be the root project so we'd just say "Your project ran without Replay Chromium" but in that case, I think that saying "Your tests ran without Replay Chromium" might still read a little better?

bvaughn · 2024-07-02T12:50:26Z

packages/playwright/src/reporter.ts

+      const projectsWithoutReplay = Object.keys(this._projects).filter(projectName => {
+        const { executed, usingReplay } = this._projects[projectName];
+        return executed && !usingReplay;
+      });


I'm not sure this is the check we want here. Shouldn't we actually be checking to ensure that at least one project using Replay ran? (It's okay if others ran that weren't using it.) e.g.

const shouldWarn = !Object.keys(this._projects).find(projectName => { const { executed, usingReplay } = this._projects[projectName]; return executed && usingReplay; });

I don't think we'd want to show ourselves this warning in the case when we chose to run with both regular Chromium and Replay Chromium, would we? (Maybe you think we would? Maybe you're worried about a case where different projects have different sets of tests they run? But that still seems pretty intentional to me.)

When I initially implemented this warning I specifically didn't want to warn when "mixed" projects would be executed... and I just forgot about this intention when working on this. I think you are right here. This also addresses the other concern that you raised

packages/playwright/src/reporter.ts

bvaughn · 2024-07-02T13:25:43Z

packages/playwright/src/reporter.ts

+      if (!existsSync(getRuntimePath())) {
+        mixpanelAPI.trackEvent("playwright.warning.reporter-used-without-replay-browser");


I think... putting this warning here doesn't make sense, does it? I was planning to follow up on this in PRO-751 but...wouldn't we error much earlier on when we tried to run the first test but there was no browser to run it with?

wouldn't we error much earlier on when we tried to run the first test but there was no browser to run it with?

Not quite. We can't check what projects are actually executed in the constructor. So we start tracking this information in onBegin but we also can't check what projects are actually executed there... so we only gain that knowledge in onEnd as at that time we know about all executed tests and related projects.

You are right that all related tests would just fail in such a scenario. It would be a Playwright-generated error though, smth like:

1) [replay-chromium] › test/example.spec.ts:3:5 › basic test ───────────────────────────────────── Error: browserType.launch: Failed to launch chromium because executable doesn't exist at /Users/mateuszburzynskiprev/.replay/runtimes/Replay-Chromium.app/Contents/MacOS/Chromium

And when that gets logged we can't add a mixpanel event.

Okay. I was thinking we could perhaps detect and warn earlier (in onTestBegin) but maybe it's better to print the error at the end anyway.

That being said, this check doesn't look right because it's warning unconditionally (even if Replay project wasn't used)

That being said, this check doesn't look right because it's warning unconditionally (even if Replay project wasn't used)

Hm, yee - you are right. It makes sense to only warn about this on runs when Replay was meant to be executed.

…sing replay

bvaughn · 2024-07-02T13:57:50Z

packages/playwright/src/reporter.ts

+      }
+
+      if (executedProjectWithReplay && !existsSync(getRuntimePath())) {
+        mixpanelAPI.trackEvent("playwright.warning.reporter-used-without-replay-browser");


I think this sounds too similar to "playwright.warning.reporter-used-without-replay-project", particularly since Playwright seems to use "project" and "browser" interchangeably

Maybe we could make this more explicit? Something like?

Suggested change

mixpanelAPI.trackEvent("playwright.warning.reporter-used-without-replay-browser");

mixpanelAPI.trackEvent("playwright.warning.replay-browser-not-installed");

Also should this be a warning or an error? I guess warning is okay?

Also should this be a warning or an error? I guess warning is okay?

It depends. If no projects with Replay were selected then this could be a warning - otherwise an error. I don't feel like complicating it this way right now. It wouldn't be thrown here anyway.

bvaughn · 2024-07-02T13:59:20Z

packages/playwright/src/reporter.ts

+
+      if (executedProjectWithReplay && !existsSync(getRuntimePath())) {
+        mixpanelAPI.trackEvent("playwright.warning.reporter-used-without-replay-browser");
+        if (output.length) {


This length check is not meaningful with how the code is currently structured. This is essentially already in an "else" block.

i became meaningful again with the recent change

i became meaningful again with the recent change

So happy for you 🥺

packages/playwright/src/reporter.ts

Co-authored-by: Brian Vaughn <brian.david.vaughn@gmail.com>

bvaughn · 2024-07-02T15:42:12Z

packages/playwright/src/reporter.ts

+      }
+
+      if (!existsSync(getRuntimePath())) {
+        mixpanelAPI.trackEvent("playwright.warning.replay-browser-not-installed");


I think the only thing I would tweak about this maybe is to conditionally report this error (if executedProjectWithReplay) so we don't get a bunch of false positives in Mixpanel?

good idea, i pushed out this change

Improve the warning about Replay-less executions

fb83220

Andarist requested a review from bvaughn July 2, 2024 12:26

Andarist commented Jul 2, 2024

View reviewed changes

Andarist changed the base branch from main to replay-chromium-warning July 2, 2024 12:35

Andarist changed the base branch from replay-chromium-warning to main July 2, 2024 12:40

bvaughn reviewed Jul 2, 2024

View reviewed changes

Andarist added 3 commits July 2, 2024 15:14

Merge remote-tracking branch 'origin/main' into fix-replayless-detection

c13caf0

tweak warning

ed48b08

Merge remote-tracking branch 'origin/main' into fix-replayless-detection

988c5a5

bvaughn mentioned this pull request Jul 2, 2024

Change formatting for browser warning #584

Merged

bvaughn reviewed Jul 2, 2024

View reviewed changes

packages/playwright/src/reporter.ts Show resolved Hide resolved

bvaughn reviewed Jul 2, 2024

View reviewed changes

only check if the browser was installed when a project was executed u…

2b049a5

…sing replay

bvaughn reviewed Jul 2, 2024

View reviewed changes

packages/playwright/src/reporter.ts Outdated Show resolved Hide resolved

bvaughn reviewed Jul 2, 2024

View reviewed changes

packages/playwright/src/reporter.ts Outdated Show resolved Hide resolved

Andarist and others added 4 commits July 2, 2024 16:27

Update packages/playwright/src/reporter.ts

2636c35

Co-authored-by: Brian Vaughn <brian.david.vaughn@gmail.com>

Update packages/playwright/src/reporter.ts

bb805e2

Update packages/playwright/src/reporter.ts

7b4fd7c

Co-authored-by: Brian Vaughn <brian.david.vaughn@gmail.com>

fmt

06f6055

bvaughn approved these changes Jul 2, 2024

View reviewed changes

avoid redundant mixpanel calls

0e55651

Andarist merged commit 543dfc9 into main Jul 2, 2024
7 checks passed

Andarist deleted the fix-replayless-detection branch July 2, 2024 16:16

github-actions bot mentioned this pull request Jul 2, 2024

Version Packages #579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the warning about Replay-less executions #585

Improve the warning about Replay-less executions #585

Andarist commented Jul 2, 2024

changeset-bot bot commented Jul 2, 2024 •

edited

Loading

Andarist Jul 2, 2024

replay-io bot commented Jul 2, 2024 •

edited

Loading

bvaughn Jul 2, 2024

bvaughn Jul 2, 2024

Andarist Jul 2, 2024

bvaughn Jul 2, 2024

Andarist Jul 2, 2024

bvaughn Jul 2, 2024

Andarist Jul 2, 2024

bvaughn Jul 2, 2024

Andarist Jul 2, 2024

bvaughn Jul 2, 2024

Andarist Jul 2, 2024

bvaughn Jul 2, 2024

Andarist Jul 2, 2024

bvaughn Jul 2, 2024

Andarist Jul 2, 2024

		if (!existsSync(getRuntimePath())) {
		mixpanelAPI.trackEvent("playwright.warning.reporter-used-without-replay-browser");

Improve the warning about Replay-less executions #585

Improve the warning about Replay-less executions #585

Conversation

Andarist commented Jul 2, 2024

changeset-bot bot commented Jul 2, 2024 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

replay-io bot commented Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Jul 2, 2024 •

edited

Loading

replay-io bot commented Jul 2, 2024 •

edited

Loading