Correctly identify test executions by utilizing `repeatEachIndex` #536

Andarist · 2024-06-17T15:25:02Z

No description provided.

changeset-bot · 2024-06-17T15:25:06Z

🦋 Changeset detected

Latest commit: e43bde8

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

This PR includes changesets to release 6 packages

Name	Type
@replayio/test-utils	Major
replayio	Minor
@replayio/playwright	Minor
@replayio/cypress	Patch
@replayio/jest	Patch
@replayio/puppeteer	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

replay-io · 2024-06-17T15:29:03Z

Status	Complete ↗︎
Commit	`c1a20fc`
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

packages/replay/src/metadata/test/v2.ts

packages/test-utils/src/reporter.ts

…ort-repeateach-in-playwright

Andarist · 2024-06-18T14:21:39Z

/release-pr

Andarist · 2024-06-18T14:57:43Z

@bvaughn @hbenl it's good to be reviewed now

bvaughn · 2024-06-18T16:46:42Z

packages/playwright/src/reporter.ts

+  // TODO: this could be simplified to `${test.testId}-${test.repeatEachIndex}-${test.attempt}`
+  // before doing that all recipients of `TestExecutionIdData` should be rechecked to see if such a change would be safe


Can you file a Linear follow-up issue for this and update the TODO comment to reference it?

packages/test-utils/src/reporter.ts

bvaughn · 2024-06-18T16:50:45Z

packages/test-utils/src/reporter.ts

+        // even when `minimizeUploads` is combined with `repeatEach` it always makes sense to upload the latest result
+        // otherwise, we could upload a single successful attempt without uploading a potential failure of the same test
+        // coming from a different `repeatEachIndex`
+        toUpload = this._minimizeUploads ? [latestExecution] : executions;


This looks wrong to me.

What if the test was flaky? We're only getting the passing recording this way.

I wonder if this should be rewritten so that we have the pass/fail/flaky check on the outside and then the minimize check within?

What if the test was flaky? We're only getting the passing recording this way.

You are correct. I'll fix this.

Something like this?

const status = computeStatus(); switch (status) { case "failed": { switch (this._uploadStatusThreshold) { case "all": case "failed": case "failed-and-flaky": { toUpload = this._minimizeUploads ? [latestExecution] : executions; break; } } break; } case "flaky": { switch (this._uploadStatusThreshold) { case "all": case "failed-and-flaky": { if (this._minimizeUploads) { toUpload = [executions[0], executions[executions.length - 1]]; } else { toUpload = executions; } break; } } break; } case "pass": { switch (this._uploadStatusThreshold) { case "all": { toUpload = this._minimizeUploads ? [latestExecution] : executions; break; } } break; } }

This function uses executions from a single execution group (that currently is equivalent to repeatEachIndex) but the uploads are minimized per test id.

We'd have an easier time if we'd push this whole decision until onEnd (that runs after everything gets completed). I decided to keep it in onTestEnd to start uploads asap - so we only have to await those upload jobs in onEnd. Maybe it's not worth it but it felt nice to have it "paralellized".

We can't determine if a test case is flaky based on a signal execution group because both could have only single executions and each of them could be of a different status. I thought about having an aggregate status on this uploadable result but it felt like a complication at a time.

The version above misses that those flakes can be spread across execution groups. I like your suggestion to reorganize this, I'll try to come up with something better.

I have rewritten it based on ur suggestion. I think it's much cleaner now but the end result isn't exactly the same as your snippet, could u take another look?

Co-authored-by: Brian Vaughn <[email protected]>

packages/test-utils/src/reporter.ts

Andarist · 2024-06-18T20:05:05Z

packages/test-utils/src/reporter.ts

+          result.didUploadStatuses.failed ||= executions.some(r => r.result !== "passed");
+          result.didUploadStatuses.passed ||= executions.some(r => r.result === "passed");
+
+          // currently previously completed execution groups that could be entirely passed or failed are not retroactively uploaded here


I don't think it's particularly important to "fix" this right now but I could certainly change my mind.

If we have such execution groups, we detect flake when the third group completes:

[passed] [passed] [failed, passed]

We could look for all completed groups and upload them but it doesn't feel overly important to me.

bvaughn

I think this needs to handle the "none" case as mentioned

bvaughn · 2024-06-18T20:23:29Z

packages/test-utils/src/reporter.ts

+        if (!this._minimizeUploads) {
+          result.didUploadStatuses.failed = true;
+          toUpload.push(...executions);
+          break;
+        }
+        if (result.didUploadStatuses.failed) {
+          break;
+        }
+        result.didUploadStatuses.failed = true;
+        toUpload.push(latestExecution);


I think we need to check if status threshold is "none" here before uploading?

Suggested change

if (!this._minimizeUploads) {

result.didUploadStatuses.failed = true;

toUpload.push(...executions);

break;

}

if (result.didUploadStatuses.failed) {

break;

}

result.didUploadStatuses.failed = true;

toUpload.push(latestExecution);

if (this._uploadStatusThreshold !== "none") {

if (this._minimizeUploads) {

if (!result.didUploadStatuses.failed) {

result.didUploadStatuses.failed = true;

toUpload.push(latestExecution);

}

} else {

result.didUploadStatuses.failed = true;

toUpload.push(...executions);

}

}

TBH I guess we could just return early from this function if that's the status because the "flaky" branch below also needs to respect that condition.

bvaughn · 2024-06-18T20:24:56Z

packages/test-utils/src/reporter.ts

-            toUpload = [latestResult];
+      }
+      case "flaky": {
+        if (this._uploadStatusThreshold === "failed") {


Unless we return early from this function I think we also need to check for "none" here too

bvaughn · 2024-06-18T20:26:18Z

packages/test-utils/src/reporter.ts

+
+          if (passedExecution) {
+            result.didUploadStatuses.failed = true;
+            toUpload.push(passedExecution);


The way this code is organized is a little hard for me to read but I think it looks correct.

bvaughn · 2024-06-18T20:28:22Z

packages/test-utils/src/reporter.ts

+      case "passed": {
+        if (this._uploadStatusThreshold !== "all") {
+          break;
+        }
+        if (!this._minimizeUploads) {
+          result.didUploadStatuses.passed = true;
+          toUpload.push(...executions);
+          break;
+        }
+        if (result.didUploadStatuses.passed) {
+          break;
        }
+        result.didUploadStatuses.passed = true;
+        toUpload.push(latestExecution);
        break;


I find interleaved break statements a little harder to read. I think it's because it feels like more work to have to keep track of all of the handled conditions in my mind as I read further along. IF/ELSE blocks feel like they provide scaffolding to make that easier.

Andarist · 2024-06-18T21:16:23Z

I think this needs to handle the "none" case as mentioned

We never get to those functions with this threshold as there is an early return here:

replay-cli/packages/test-utils/src/reporter.ts

Lines 912 to 914 in be5d659

    
           if (this._uploadStatusThreshold === "none") { 
        
             return; 
        
           }

bvaughn · 2024-06-18T21:58:42Z

I think this needs to handle the "none" case as mentioned

We never get to those functions with this threshold as there is an early return here:

replay-cli/packages/test-utils/src/reporter.ts

Lines 912 to 914 in be5d659

if (this._uploadStatusThreshold === "none") {

return;

}

I don't think that's a strong enough guard. This method could be called from other places (even if it isn't currently) so I think it needs its own guard. (Avoid relying on less obvious / indirect checks for things like this is a lesson I've learned for myself over the years, not to depend on indirect or less obvious short-circuits for things like this.)

Correctly identify test executions by utilizing repeatEachIndex

9508a0b

tweak advanced upload handling to accomodate for repeatEach

169b844

Andarist commented Jun 17, 2024

View reviewed changes

packages/replay/src/metadata/test/v2.ts Outdated Show resolved Hide resolved

Andarist added 3 commits June 17, 2024 18:10

Fixed projectName

daf12be

avoid spreading redundant props

c70f348

avoid reuploading the same recording

2fe5575

Andarist marked this pull request as ready for review June 17, 2024 16:45

Andarist requested review from bvaughn and hbenl June 17, 2024 16:45

Andarist commented Jun 17, 2024

View reviewed changes

packages/test-utils/src/reporter.ts Outdated Show resolved Hide resolved

Andarist added 4 commits June 18, 2024 13:48

Merge remote-tracking branch 'origin/main' into andarist/pro-611-supp…

bfcf0bf

…ort-repeateach-in-playwright

revert major schema bump

f1a17ee

include filepaths in executionIds

79bb5f0

retract and use execution groups

f2c530f

update metadata structs in the new cli

21666cb

bvaughn reviewed Jun 18, 2024

View reviewed changes

packages/test-utils/src/reporter.ts Outdated Show resolved Hide resolved

bvaughn reviewed Jun 18, 2024

View reviewed changes

Andarist and others added 3 commits June 18, 2024 19:19

add changesets

716889f

Update packages/test-utils/src/reporter.ts

a8a2a1b

Co-authored-by: Brian Vaughn <[email protected]>

fixed minimized flaky updates with all threshold

a039dae

bvaughn reviewed Jun 18, 2024

View reviewed changes

packages/test-utils/src/reporter.ts Outdated Show resolved Hide resolved

bvaughn reviewed Jun 18, 2024

View reviewed changes

packages/test-utils/src/reporter.ts Show resolved Hide resolved

Andarist added 2 commits June 18, 2024 21:57

cleanup the upload selection logic

72d1660

always update didUploadStatuses

be5d659

Andarist commented Jun 18, 2024

View reviewed changes

bvaughn approved these changes Jun 18, 2024

View reviewed changes

Andarist added 2 commits June 19, 2024 12:15

make sure that the same recording is not uloaded more than once

8a2f377

add early return to _enqueueUploads

e43bde8

Andarist merged commit 8343abe into main Jun 19, 2024
7 checks passed

Andarist deleted the andarist/pro-611-support-repeateach-in-playwright branch June 19, 2024 10:18

github-actions bot mentioned this pull request Jun 19, 2024

Version Packages #521

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly identify test executions by utilizing `repeatEachIndex` #536

Correctly identify test executions by utilizing `repeatEachIndex` #536

Andarist commented Jun 17, 2024

changeset-bot bot commented Jun 17, 2024 •

edited

Loading

replay-io bot commented Jun 17, 2024 •

edited

Loading

Andarist commented Jun 18, 2024

Andarist commented Jun 18, 2024

bvaughn Jun 18, 2024

Andarist Jun 18, 2024

bvaughn Jun 18, 2024

bvaughn Jun 18, 2024

Andarist Jun 18, 2024

bvaughn Jun 18, 2024

Andarist Jun 18, 2024

Andarist Jun 18, 2024

Andarist Jun 18, 2024

bvaughn left a comment

bvaughn Jun 18, 2024

bvaughn Jun 18, 2024

bvaughn Jun 18, 2024

bvaughn Jun 18, 2024 •

edited

Loading

Andarist commented Jun 18, 2024

bvaughn commented Jun 18, 2024 •

edited

Loading

		// TODO: this could be simplified to `${test.testId}-${test.repeatEachIndex}-${test.attempt}`
		// before doing that all recipients of `TestExecutionIdData` should be rechecked to see if such a change would be safe

Correctly identify test executions by utilizing repeatEachIndex #536

Correctly identify test executions by utilizing repeatEachIndex #536

Conversation

Andarist commented Jun 17, 2024

changeset-bot bot commented Jun 17, 2024 • edited Loading

🦋 Changeset detected

replay-io bot commented Jun 17, 2024 • edited Loading

Andarist commented Jun 18, 2024

Andarist commented Jun 18, 2024

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

bvaughn left a 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

bvaughn Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Andarist commented Jun 18, 2024

bvaughn commented Jun 18, 2024 • edited Loading

Correctly identify test executions by utilizing `repeatEachIndex` #536

Correctly identify test executions by utilizing `repeatEachIndex` #536

changeset-bot bot commented Jun 17, 2024 •

edited

Loading

replay-io bot commented Jun 17, 2024 •

edited

Loading

bvaughn Jun 18, 2024 •

edited

Loading

bvaughn commented Jun 18, 2024 •

edited

Loading