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

Fixed displayError being incorrectly used for all tests attempts #530

Merged
merged 3 commits into from
Jun 13, 2024
Merged

Fixed displayError being incorrectly used for all tests attempts #530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
14d64ba
Fixed `displayError` being incorrectly used for all tests attempts
Andarist Jun 13, 2024
4b0a09a
add todo comment
Andarist Jun 13, 2024
cc5e1ac
add changesets
Andarist Jun 13, 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/gentle-lamps-taste.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@replayio/cypress": major
---

Bumped the required peer dependency range on Cypress to `^13`
5 changes: 5 additions & 0 deletions .changeset/thirty-tools-boil.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@replayio/cypress": patch
---

Fixed an issue with `displayError` being incorrectly reused for all test attempts
2 changes: 1 addition & 1 deletion packages/cypress/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
tests/fixtures
tests/fixtures
2 changes: 1 addition & 1 deletion packages/cypress/example/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
"@replayio/cypress": "workspace:^",
"@types/react": "^18",
"@types/react-dom": "^18",
"cypress": "^10.9.0"
"cypress": "^13.11.0"
},
"scripts": {
"start": "react-scripts start",
Expand Down
4 changes: 2 additions & 2 deletions packages/cypress/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
"@types/semver": "^7.3.13",
"@types/uuid": "^9.0.1",
"@types/ws": "^8.5.10",
"cypress": "^10.9.0",
"cypress": "^13.11.0",
"typescript": "^5.4.5"
},
"dependencies": {
Expand All @@ -54,6 +54,6 @@
"ws": "^8.14.2"
},
"peerDependencies": {
"cypress": ">=5.3.0"
"cypress": "^13"
Copy link
Member Author

Choose a reason for hiding this comment

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

Cypress 13 was released almost one year ago (08/29/2023). We don't have any tests for this whole integration, trying to test compatibility manually with an extended range of versions is a nightmare. I don't feel we can quite guarantee compatibility right now with other versions, it's impractical.

If requested by clients we can always loosen up this range after manually confirming that a particular combination works. We'll release a new major of this package soon so I think we should grab the opportunity and bump this here.

Note that supporting more right now would require some extra work here, I changed this because I ran into typing problems (that likely were related to breaking changes in Cypress).

Copy link
Contributor

Choose a reason for hiding this comment

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

I support this.

}
}
2 changes: 1 addition & 1 deletion packages/cypress/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ async function onBeforeRun(details: Cypress.BeforeRunDetails) {

function onBeforeBrowserLaunch(
browser: Cypress.Browser,
launchOptions: Cypress.BrowserLaunchOptions
launchOptions: Cypress.BeforeBrowserLaunchOptions
) {
debugEvents("Handling before:browser:launch");
assertReporter(cypressReporter);
Expand Down
26 changes: 18 additions & 8 deletions packages/cypress/src/steps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,13 @@ function getTestsFromResults(
const tests = resultTests.flatMap<Test>((result, id) => {
const scope = [...result.title];
const title = scope.pop()!;
const lastAttemptIndex = result.attempts.length - 1;

return result.attempts.map<Test>((a, attemptIndex) => ({
id: getIdForTest(result) ?? attemptIndex,
// Cypress 10.9 types are wrong here ... duration doesn't exist but wallClockDuration does
approximateDuration: a.duration || (a as any).wallClockDuration || 0,
// those properties don't exist since Cypress 13: https://github.com/cypress-io/cypress/pull/27230
// TODO: remove it in PRO-640
approximateDuration: (a as any).duration || (a as any).wallClockDuration || 0,
Copy link
Member Author

Choose a reason for hiding this comment

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

With the latest version of Cypress this is non-sensical. It will just always be 0. I'm afraid to remove it though, without confirming that devtools and dashboard would be fine without this

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a Linear task + TODO comment to follow up here? Else it will get forgotten about.

Copy link
Member Author

Choose a reason for hiding this comment

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

done, PRO-640

attempt: attemptIndex + 1,
source: {
title,
Expand All @@ -96,12 +98,20 @@ function getTestsFromResults(
afterEach: [],
main: [],
},
error: result.displayError
? {
name: "DisplayError",
message: result.displayError,
}
: null,
// attempt.error is available here:
// https://github.com/cypress-io/cypress/blob/61156808413be8b99264026323ce3abfb22c4c26/packages/server/lib/modes/results.ts#L20
// but it's lost when creating a public test result:
// https://github.com/cypress-io/cypress/blob/61156808413be8b99264026323ce3abfb22c4c26/packages/server/lib/modes/results.ts#L111
// `.displayError` represents the error reported by the last attempt
// for all of the previous attempts we rely on the search for the last errored step in `groupStepsByTest`
// if an error is found there, it will be set on the test, the information set here is not overriden though
Comment on lines +105 to +107
Copy link
Member Author

Choose a reason for hiding this comment

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

This logic is a little bit wonky but it makes sense. The .displayError is something computed by Cypress so it should have higher accuracy than what we can compute on our own. OTOH, we have to compute this on our own for non-last attempts because Cypress doesn't expose this information to us.

It would be great to refactor this so we don't have to rely on a mutation elsewhere but that's out of this PR's scope

error:
attemptIndex === lastAttemptIndex && result.displayError
? {
name: "DisplayError",
message: result.displayError,
}
: null,
}));
});

Expand Down
12 changes: 6 additions & 6 deletions packages/cypress/src/support.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,15 +82,15 @@ function handleReplayConnectResponse(v: unknown) {
}
}

function isReplayConnectCallbackCommand(cmd: Cypress.EnqueuedCommand) {
function isReplayConnectCallbackCommand(cmd: Cypress.EnqueuedCommandAttributes) {
return (
cmd.name === "then" && Array.isArray(cmd.args) && cmd.args[0] === handleReplayConnectResponse
);
}

function shouldIgnoreCommand(cmd: Cypress.EnqueuedCommand | Cypress.CommandQueue) {
function shouldIgnoreCommand(cmd: Cypress.EnqueuedCommandAttributes | Cypress.CommandQueue) {
if (isCommandQueue(cmd)) {
cmd = cmd.toJSON() as any as Cypress.EnqueuedCommand;
cmd = cmd.toJSON() as any as Cypress.EnqueuedCommandAttributes;
}

if (isReplayConnectCallbackCommand(cmd)) {
Expand Down Expand Up @@ -444,7 +444,7 @@ function register() {
// covert it using its toJSON method (which is typed wrong so we have to
// cast it to any first)
if (isCommandQueue(cmd)) {
cmd = cmd.toJSON() as any as Cypress.EnqueuedCommand;
cmd = cmd.toJSON() as any as Cypress.EnqueuedCommandAttributes;
}

const id = getReplayId(
Expand Down Expand Up @@ -579,7 +579,7 @@ function register() {
logs: () => [],
add: () => {},
get: (key?: any) => (!key ? log : log[key]),
toJSON: () => log,
toJSON: () => [],
Copy link
Member Author

Choose a reason for hiding this comment

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

the previous version was completely crashing Cypress, the toJSON's type here expects string[] so I used an empty array to ensure that it's compatible. In Cypress 10 log was typed as any and maybe at that time it worked OK, dunno. Today it's an object so we can see how it isn't compatible with the expected string[].

I verified that we can still render cy.log just fine with this. It's quite plausible that we just don't rely on the return value of this function.

create: () => ({} as any),
};
} else if (maybeCurrentAssertion?.get("type") !== "assertion") {
Expand All @@ -591,7 +591,7 @@ function register() {
return;
}

if (shouldIgnoreCommand(maybeCurrentAssertion)) {
if (!maybeCurrentAssertion || shouldIgnoreCommand(maybeCurrentAssertion)) {
return;
}

Expand Down
76 changes: 45 additions & 31 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2246,9 +2246,9 @@ __metadata:
languageName: node
linkType: hard

"@cypress/request@npm:^2.88.10":
version: 2.88.12
resolution: "@cypress/request@npm:2.88.12"
"@cypress/request@npm:^3.0.0":
version: 3.0.1
resolution: "@cypress/request@npm:3.0.1"
dependencies:
aws-sign2: "npm:~0.7.0"
aws4: "npm:^1.8.0"
Expand All @@ -2263,12 +2263,12 @@ __metadata:
json-stringify-safe: "npm:~5.0.1"
mime-types: "npm:~2.1.19"
performance-now: "npm:^2.1.0"
qs: "npm:~6.10.3"
qs: "npm:6.10.4"
safe-buffer: "npm:^5.1.2"
tough-cookie: "npm:^4.1.3"
tunnel-agent: "npm:^0.6.0"
uuid: "npm:^8.3.2"
checksum: 10c0/815d56db900d63479a7385f1f76d583f1fea6181499ed8abd54aec477423b03202e1155389e0035c3ec29852a0cce39c21629f358b604f604d4653b6d0ce9d52
checksum: 10c0/8eb92a665e6549e2533f5169431addcaad0307f51a8c7f3b6b169eb79b4d673373784a527590a47b0a2905ad5f601b24ab2d1b31d184243235aba470ffc9c1f7
languageName: node
linkType: hard

Expand Down Expand Up @@ -3232,7 +3232,7 @@ __metadata:
"@types/react": "npm:^18"
"@types/react-dom": "npm:^18"
cra-template-typescript: "npm:1.2.0"
cypress: "npm:^10.9.0"
cypress: "npm:^13.11.0"
react: "npm:^18.2.0"
react-dom: "npm:^18.2.0"
react-scripts: "npm:5.0.1"
Expand All @@ -3253,7 +3253,7 @@ __metadata:
"@types/uuid": "npm:^9.0.1"
"@types/ws": "npm:^8.5.10"
chalk: "npm:^4.1.2"
cypress: "npm:^10.9.0"
cypress: "npm:^13.11.0"
debug: "npm:^4.3.4"
semver: "npm:^7.5.2"
terminate: "npm:^2.6.1"
Expand All @@ -3262,7 +3262,7 @@ __metadata:
uuid: "npm:^8.3.2"
ws: "npm:^8.14.2"
peerDependencies:
cypress: ">=5.3.0"
cypress: ^13
languageName: unknown
linkType: soft

Expand Down Expand Up @@ -5850,7 +5850,7 @@ __metadata:
languageName: node
linkType: hard

"buffer@npm:^5.5.0, buffer@npm:^5.6.0":
"buffer@npm:^5.5.0, buffer@npm:^5.7.1":
version: 5.7.1
resolution: "buffer@npm:5.7.1"
dependencies:
Expand Down Expand Up @@ -6383,6 +6383,13 @@ __metadata:
languageName: node
linkType: hard

"commander@npm:^6.2.1":
version: 6.2.1
resolution: "commander@npm:6.2.1"
checksum: 10c0/85748abd9d18c8bc88febed58b98f66b7c591d9b5017cad459565761d7b29ca13b7783ea2ee5ce84bf235897333706c4ce29adf1ce15c8252780e7000e2ce9ea
languageName: node
linkType: hard

"commander@npm:^7.2.0":
version: 7.2.0
resolution: "commander@npm:7.2.0"
Expand Down Expand Up @@ -6945,28 +6952,27 @@ __metadata:
languageName: node
linkType: hard

"cypress@npm:^10.9.0":
version: 10.11.0
resolution: "cypress@npm:10.11.0"
"cypress@npm:^13.11.0":
version: 13.11.0
resolution: "cypress@npm:13.11.0"
dependencies:
"@cypress/request": "npm:^2.88.10"
"@cypress/request": "npm:^3.0.0"
"@cypress/xvfb": "npm:^1.2.4"
"@types/node": "npm:^14.14.31"
"@types/sinonjs__fake-timers": "npm:8.1.1"
"@types/sizzle": "npm:^2.3.2"
arch: "npm:^2.2.0"
blob-util: "npm:^2.0.2"
bluebird: "npm:^3.7.2"
buffer: "npm:^5.6.0"
buffer: "npm:^5.7.1"
cachedir: "npm:^2.3.0"
chalk: "npm:^4.1.0"
check-more-types: "npm:^2.24.0"
cli-cursor: "npm:^3.1.0"
cli-table3: "npm:~0.6.1"
commander: "npm:^5.1.0"
commander: "npm:^6.2.1"
common-tags: "npm:^1.8.0"
dayjs: "npm:^1.10.4"
debug: "npm:^4.3.2"
debug: "npm:^4.3.4"
enquirer: "npm:^2.3.6"
eventemitter2: "npm:6.4.7"
execa: "npm:4.1.0"
Expand All @@ -6975,25 +6981,26 @@ __metadata:
figures: "npm:^3.2.0"
fs-extra: "npm:^9.1.0"
getos: "npm:^3.2.1"
is-ci: "npm:^3.0.0"
is-ci: "npm:^3.0.1"
is-installed-globally: "npm:~0.4.0"
lazy-ass: "npm:^1.6.0"
listr2: "npm:^3.8.3"
lodash: "npm:^4.17.21"
log-symbols: "npm:^4.0.0"
minimist: "npm:^1.2.6"
minimist: "npm:^1.2.8"
ospath: "npm:^1.2.2"
pretty-bytes: "npm:^5.6.0"
process: "npm:^0.11.10"
proxy-from-env: "npm:1.0.0"
request-progress: "npm:^3.0.0"
semver: "npm:^7.3.2"
semver: "npm:^7.5.3"
supports-color: "npm:^8.1.1"
tmp: "npm:~0.2.1"
untildify: "npm:^4.0.0"
yauzl: "npm:^2.10.0"
bin:
cypress: bin/cypress
checksum: 10c0/0a22163a3f75422e8d972fd29e777c1a2b5915144d5aab1abf32ff29e0c8f771c74d61da64aea5dba9d305ceb194c2afe4ea7a50a09bda95ec66842e163a2bc2
checksum: 10c0/a78eca7c26279928a86110d136a8ffcb339e81a04345eff155b0bc1b58f39bcce669f72f9d4e8303d038daf477525e727be2e1814ae04c100a3700c5f6f922f2
languageName: node
linkType: hard

Expand Down Expand Up @@ -10072,7 +10079,7 @@ __metadata:
languageName: node
linkType: hard

"is-ci@npm:^3.0.0":
"is-ci@npm:^3.0.1":
version: 3.0.1
resolution: "is-ci@npm:3.0.1"
dependencies:
Expand Down Expand Up @@ -12430,7 +12437,7 @@ __metadata:
languageName: node
linkType: hard

"minimist@npm:^1.2.0, minimist@npm:^1.2.6":
"minimist@npm:^1.2.0, minimist@npm:^1.2.6, minimist@npm:^1.2.8":
version: 1.2.8
resolution: "minimist@npm:1.2.8"
checksum: 10c0/19d3fcdca050087b84c2029841a093691a91259a47def2f18222f41e7645a0b7c44ef4b40e88a1e58a40c84d2ef0ee6047c55594d298146d0eb3f6b737c20ce6
Expand Down Expand Up @@ -14341,6 +14348,13 @@ __metadata:
languageName: node
linkType: hard

"process@npm:^0.11.10":
version: 0.11.10
resolution: "process@npm:0.11.10"
checksum: 10c0/40c3ce4b7e6d4b8c3355479df77aeed46f81b279818ccdc500124e6a5ab882c0cc81ff7ea16384873a95a74c4570b01b120f287abbdd4c877931460eca6084b3
languageName: node
linkType: hard

"progress@npm:^2.0.0":
version: 2.0.3
resolution: "progress@npm:2.0.3"
Expand Down Expand Up @@ -14474,21 +14488,21 @@ __metadata:
languageName: node
linkType: hard

"qs@npm:6.11.0":
version: 6.11.0
resolution: "qs@npm:6.11.0"
"qs@npm:6.10.4":
version: 6.10.4
resolution: "qs@npm:6.10.4"
dependencies:
side-channel: "npm:^1.0.4"
checksum: 10c0/4e4875e4d7c7c31c233d07a448e7e4650f456178b9dd3766b7cfa13158fdb24ecb8c4f059fa91e820dc6ab9f2d243721d071c9c0378892dcdad86e9e9a27c68f
checksum: 10c0/7a8c9d77968aeccb769aedd7e047c0e0109dad0cfa57cab1ad906f4069fd58f361b80abd2de5854ba9a09b4c5d06d6a2c82108766f1f1527572fe6130deaa471
languageName: node
linkType: hard

"qs@npm:~6.10.3":
version: 6.10.4
resolution: "qs@npm:6.10.4"
"qs@npm:6.11.0":
version: 6.11.0
resolution: "qs@npm:6.11.0"
dependencies:
side-channel: "npm:^1.0.4"
checksum: 10c0/7a8c9d77968aeccb769aedd7e047c0e0109dad0cfa57cab1ad906f4069fd58f361b80abd2de5854ba9a09b4c5d06d6a2c82108766f1f1527572fe6130deaa471
checksum: 10c0/4e4875e4d7c7c31c233d07a448e7e4650f456178b9dd3766b7cfa13158fdb24ecb8c4f059fa91e820dc6ab9f2d243721d071c9c0378892dcdad86e9e9a27c68f
languageName: node
linkType: hard

Expand Down