chore: fix tests that fail on windows #21055

lmiller1990 · 2022-04-12T22:42:59Z

User facing changelog

Fixes some flaky tests.

Additional details

fixes some runner mocha event tests w/ a slightly longer timeout
fixes an issue where git info wasn't working correctly due to / vs \ on different OSs
fixes race condition where could not connect to CDP when launching with cypress --browser chrome which primarily manifested on windows

How has the user experience changed?

Tests pass on windows.

PR Tasks

Have tests been added/updated?
Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
Has a PR for user-facing changes been opened in cypress-documentation?
Have API changes been updated in the type definitions?
Have new configuration options been added to the cypress.schema.json?

cypress-bot · 2022-04-12T22:43:02Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

lmiller1990 · 2022-04-12T22:49:24Z

packages/data-context/src/sources/GitDataSource.ts


    // windows returns a leading carriage return, remove it
-    const [, ...stdout] = split
+    let output: string[] = []


More reliable way to pass shell command output.

lmiller1990 · 2022-04-12T23:05:42Z

packages/data-context/src/sources/GitDataSource.ts

-    const split = result.stdout
-    .split('\r\n') // windows uses CRLF for carriage returns
-    .filter((str) => !str.includes('git log')) // windows stdout contains [cmd,output]. So we remove the code containing the executed command, `git log`
+    const stdout = normalize(result.stdout).split('\r\n') // windows uses CRLF for carriage returns


This was the problem, / vs \.

Def need to standardize how we handle this internally. At least w/ this PR we have more coverage and confidence across all OSes, for when we do try to standardize this (if we need to make changes, maybe we don't).

Yeah... maybe a guides/ would be good. Maybe a guide all about platform differences, not just paths?

In general, I think it should work like this:

We should always use the native path if it is being displayed to the user. Unless it's displayed in a place where it may be shared cross-platform in a programmatic way (like require(path), import path, or in the URL bar)

We should always use the native path internally unless the library does not support it (like in globby, where \ acts as an escape character.

What do you think?

Guides + trusted windows tests would be a good place to start. Shared utils/libs to handle the translation when necessary could help too.

cypress · 2022-04-12T23:08:00Z

Test summary

4343 • 0 • 48 • 0 • 1

Run details


Project	cypress
Status	Passed
Commit	`a2a5e24`
Started	Apr 19, 2022 5:38 AM
Ended	Apr 19, 2022 5:51 AM
Duration	13:34 💡
OS	Linux Debian - 10.10
Browser	Electron 94

View run in Cypress Dashboard ➡️

Flakiness

	cypress/e2e/commands/net_stubbing.cy.ts		1
1	network stubbing > intercepting request > can delay and throttle a StaticResponse

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

packages/data-context/src/sources/GitDataSource.ts

flotwig · 2022-04-13T16:37:22Z

packages/data-context/src/sources/GitDataSource.ts

-    const split = result.stdout
-    .split('\r\n') // windows uses CRLF for carriage returns
-    .filter((str) => !str.includes('git log')) // windows stdout contains [cmd,output]. So we remove the code containing the executed command, `git log`
+    const stdout = normalize(result.stdout).split('\r\n') // windows uses CRLF for carriage returns


Yeah... maybe a guides/ would be good. Maybe a guide all about platform differences, not just paths?

In general, I think it should work like this:

We should always use the native path if it is being displayed to the user. Unless it's displayed in a place where it may be shared cross-platform in a programmatic way (like require(path), import path, or in the URL bar)

We should always use the native path internally unless the library does not support it (like in globby, where \ acts as an escape character.

What do you think?

packages/data-context/src/sources/GitDataSource.ts

tbiethman · 2022-04-13T21:57:40Z

packages/app/cypress/e2e/specs_list_actual_git_repo.cy.ts

+    // should update via GraphQL subscription, now the status is modified.
+    cy.get('[data-cy-row="dom-container.spec.js"]')
+    .contains('Modified')
+    .get('[data-cy="git-status-modified"]')


Good call on the additional coverage 👍

Co-authored-by: Zach Bloomquist <git@chary.us>

…ress into lmiller1990/windows-fails

ryanthemanuel · 2022-04-14T16:06:11Z

circle.yml

@@ -1189,6 +1189,8 @@ jobs:
    parallelism: 1
    steps:
      - restore_cached_workspace
+      - run: mkdir -p ~/.ssh


I really wish I knew why this was necessary (still looking). The problem seems to only be happening on my PRs

circle.yml

flotwig · 2022-04-14T16:44:19Z

guides/remaining-platform-agnostic.md

@@ -0,0 +1,85 @@
+# Remaining Platform Agnostic


"Writing Cross-Platform JavaScript" might be a clearer title, or something along those lines? Thanks for adding this, now we have something to point to.

guides/remaining-platform-agnostic.md

tbiethman · 2022-04-14T16:53:20Z

packages/app/src/specs/SpecsList.vue

@@ -67,7 +67,7 @@
          :id="getIdIfDirectory(row)"
          :key="row.index"
          :data-cy="row.data.isLeaf ? 'spec-list-file' : 'spec-list-directory'"
-          :data-cy-row="row.data.data?.relative"
+          :data-cy-row="row.data.data?.baseName"


Just wanted to note that using baseName here doesn't guarantee uniqueness like relative did. Doesn't seem like it impacts any current tests, though.

Yep - I think it's probably fine for now, actually updating the paths is a bit of scope creep, and changing the paths dynamically for the assertion seems like more technical debt. This seems like a reasonable trade-off.

packages/server/lib/browsers/browser-cri-client.ts

tbiethman · 2022-04-14T17:03:05Z

packages/server/lib/browsers/browser-cri-client.ts


-    // TODO: Move this into packages/error post merge: https://github.com/cypress-io/cypress/pull/20072


Is this TODO worth persisting/doing?

This fix actually negates the rationale for the TODO (which is nice). This scenarios is now handled by this catch block:

https://github.com/cypress-io/cypress/pull/21055/files#diff-b5b66d85423fa714add2e866448d3411b0f1cdf2cb4b4a95aa80394fa3edd1b3R57

which properly displays the appropriate error.

Co-authored-by: Zach Bloomquist <git@chary.us>

lmiller1990 · 2022-04-15T02:07:56Z

I have responded to the comments

updated guide
updated guide name
added some comments to explain what code does

…s-io/cypress into lmiller1990/windows-fails

guides/writing-cross-platform-javascript.md

Co-authored-by: Ryan Manuel <ryanm@cypress.io>

…e-config * 10.0-release: chore: Move component-index generation to scaffold-config package (#21090) fix: label text should be clickable to toggle snapshot highlight (#21122) feat: add next preset to webpack-dev-server-fresh (#21069) chore: add dev-servers as deps to server to be included in the binary (#21142) fix: do not highlight preExtension if selected option is renameFolder (#21121) fix(unify): Remove 'Reconfigure' dropdown from Testing Type chooser (#21063) feat(unify): launchpad header breadcrumbs and reusable tooltip component (#20648) test: add windows-test-kitchensink job (#20847) fix: aut centering and height calculation (#21019) chore: fix tests that fail on windows (#21055) fix: running a new test after already having run tests (#21087) fix: ct project setup framework keys for next and nuxt (#21116) fix: remove MountReturn from scaffolded ct support file (#21119) fix: do not scaffold fixtures if folder exist (#21078) fix: revert "fix: types for Cypress.Commands.add (#20376)" (#21104) chore: Update Chrome (stable) to 100.0.4896.127 and Chrome (beta) to 101.0.4951.34 (#21083) fix: types for Cypress.Commands.add (#20376) (#20377)

lmiller1990 added 7 commits April 12, 2022 21:48

check if failing

7d5b5a9

update timeouts

dcd9bff

move cy code inside it

9cfe481

revert code

7aac707

fixing test

eee4eef

update timeout

229635f

update tests

7299d6a

lmiller1990 mentioned this pull request Apr 12, 2022

chore: fix run-app-integration for windows #21040

Closed

lmiller1990 requested review from flotwig and tbiethman April 12, 2022 22:48

lmiller1990 commented Apr 12, 2022

View reviewed changes

lmiller1990 marked this pull request as ready for review April 12, 2022 23:03

lmiller1990 requested a review from tgriesser as a code owner April 12, 2022 23:03

lmiller1990 commented Apr 12, 2022

View reviewed changes

ryanthemanuel added 4 commits April 12, 2022 22:25

fix: handle race condition with windows and the cri client

37749a3

fix: handle race condition with windows and the cri client

97a1f3c

fix: handle race condition with windows and the cri client

d74642b

Refactor and unit tests fixes

ed7f469

flotwig suggested changes Apr 13, 2022

View reviewed changes

ryanthemanuel and others added 4 commits April 13, 2022 13:26

Further refactoring and error handling cleanup

3d57e7b

integrate other windows fixes

1661301

update test timeout

8de78a0

Update timeout for spec subscription e2e test

5969d80

tbiethman reviewed Apr 13, 2022

View reviewed changes

ryanthemanuel and others added 3 commits April 13, 2022 17:27

Fix issue with ssh keys in circle

35cbb18

Apply suggestions from code review

5ac41f4

Co-authored-by: Zach Bloomquist <git@chary.us>

Merge branch 'lmiller1990/windows-fails' of github.com:cypress-io/cyp…

1707331

…ress into lmiller1990/windows-fails

lmiller1990 requested a review from a team as a code owner April 13, 2022 23:12

Merge branch 'lmiller1990/windows-fails' of github.com:cypress-io/cyp…

6d465c8

…ress into lmiller1990/windows-fails

flotwig requested review from tbiethman and flotwig April 14, 2022 15:42

ryanthemanuel self-requested a review April 14, 2022 16:05

ryanthemanuel reviewed Apr 14, 2022

View reviewed changes

circle.yml Outdated Show resolved Hide resolved

Update circle.yml

de10a10

flotwig reviewed Apr 14, 2022

View reviewed changes

guides/remaining-platform-agnostic.md Outdated Show resolved Hide resolved

tbiethman reviewed Apr 14, 2022

View reviewed changes

packages/server/lib/browsers/browser-cri-client.ts Show resolved Hide resolved

tbiethman reviewed Apr 14, 2022

View reviewed changes

tbiethman and others added 4 commits April 14, 2022 12:04

Merge branch '10.0-release' into lmiller1990/windows-fails

b03b644

Update guides/remaining-platform-agnostic.md

aa66840

Co-authored-by: Zach Bloomquist <git@chary.us>

update docs and comments

54b7a8a

rename file

d0340ae

Merge branch 'lmiller1990/windows-fails' of https://github.com/cypres…

ab5dbd0

…s-io/cypress into lmiller1990/windows-fails

flotwig requested review from flotwig, ryanthemanuel and tbiethman April 15, 2022 15:34

Merge branch '10.0-release' into lmiller1990/windows-fails

236279c

ryanthemanuel reviewed Apr 15, 2022

View reviewed changes

guides/writing-cross-platform-javascript.md Outdated Show resolved Hide resolved

ryanthemanuel reviewed Apr 15, 2022

View reviewed changes

guides/writing-cross-platform-javascript.md Outdated Show resolved Hide resolved

lmiller1990 and others added 2 commits April 18, 2022 10:48

Apply suggestions from code review

1732d86

Co-authored-by: Ryan Manuel <ryanm@cypress.io>

Merge branch '10.0-release' into lmiller1990/windows-fails

a2a5e24

tbiethman approved these changes Apr 19, 2022

View reviewed changes

lmiller1990 merged commit 0a320d3 into 10.0-release Apr 19, 2022

lmiller1990 deleted the lmiller1990/windows-fails branch April 19, 2022 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: fix tests that fail on windows #21055

chore: fix tests that fail on windows #21055

lmiller1990 commented Apr 12, 2022 •

edited

cypress-bot bot commented Apr 12, 2022

lmiller1990 Apr 12, 2022

lmiller1990 Apr 12, 2022

flotwig Apr 13, 2022

tbiethman Apr 13, 2022

cypress bot commented Apr 12, 2022 •

edited

flotwig Apr 13, 2022

tbiethman Apr 13, 2022

ryanthemanuel Apr 14, 2022

flotwig Apr 14, 2022

lmiller1990 Apr 15, 2022

tbiethman Apr 14, 2022

lmiller1990 Apr 15, 2022

tbiethman Apr 14, 2022 •

edited

ryanthemanuel Apr 15, 2022

lmiller1990 commented Apr 15, 2022


		// TODO: Move this into packages/error post merge: https://github.com/cypress-io/cypress/pull/20072

chore: fix tests that fail on windows #21055

chore: fix tests that fail on windows #21055

Conversation

lmiller1990 commented Apr 12, 2022 • edited

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Apr 12, 2022

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

cypress bot commented Apr 12, 2022 • edited

Test summary

Run details

Flakiness

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

tbiethman Apr 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 commented Apr 15, 2022

lmiller1990 commented Apr 12, 2022 •

edited

cypress bot commented Apr 12, 2022 •

edited

tbiethman Apr 14, 2022 •

edited