test: allow more tests to run on windows #10351

G-Rath · 2020-08-01T01:05:13Z

Summary

Closes #3121 (mostly) - the only two outstanding are the yarn pnp test & an open handles test.

Test plan

See if Windows runs in CI passes after adjusting the relevant tests.

SimenB · 2020-08-01T11:12:17Z

Yay! Better coverage on Windows is wonderful

G-Rath · 2020-08-01T11:14:24Z

For sure, although jest-haste-map had me in a bit of state by the end of it... 😵😵😵

G-Rath · 2020-08-01T11:16:49Z

e2e/__tests__/detectOpenHandles.ts

  it('does not report --notify flag', () => {
+    if (process.platform === 'win32') {


This is the only test aside from the yarn pnp one that is now being skipped on Windows 🎉

Open to ideas on if anything can be done to eliminate it - I suspect it represents an actual "bug" (/ maybe just something that can't be supported?)

SimenB

Yay, thank you so much!

SimenB · 2020-08-01T11:13:29Z

e2e/__tests__/cliHandlesExactFilenames.test.ts

@@ -30,7 +27,9 @@ test('CLI accepts exact file names if matchers matched', () => {

  expect(result.exitCode).toBe(0);

-  const {rest, summary} = extractSummary(result.stderr);
+  const {rest, summary} = extractSummary(
+    result.stderr.replace('\\\\foo\\\\bar', '\\/foo\\/bar'),


Could we normalize the output of jest instead of normalizing in the test itself?

Technically, yes but I don't know how much work that'll involve.

However, I don't know if thats a good idea from UX POV, as jest is implicitly normalizing the input - we pass in ./foo/bar.spec.js, which is a Linux style file path (it's actually probably node doing the normalizing, but the result is the same).

By outputting the actual OS-specific file path that was used, users can see that it doesn't matter if they pass in Linux file paths; where as if we normalised them in the output here, personally I'd look at that and think "oh maybe Jest isn't finding my test file because it's using the Linux file path" (especially since copying & pasting the file path into Windows would cause an error, as iirc Powershell, CMD, and Explorer all expect forward slashes).

e2e/__tests__/dependencyClash.test.ts

SimenB · 2020-08-01T11:15:35Z

e2e/__tests__/detectOpenHandles.ts

  it('does not report --notify flag', () => {
+    if (process.platform === 'win32') {


Isn't this the same as the skip?

No, as the skip causes all tests in the file to be skipped, but this is the only one that actually fails.

e2e/__tests__/pnp.test.ts

SimenB · 2020-08-01T11:18:00Z

packages/test-utils/src/ConditionalTest.ts

@@ -29,14 +29,6 @@ export function skipSuiteOnJestCircus(): void {
  }
 }

-export function skipSuiteOnWindows(): void {


Keeping this until all skips are resolved seems better to me - easier to find usage 🙂

It was in part for the excitement of it - the two tests that are remaining don't seem like they're going to go away anytime soon (the yarn pnp one at least), and I felt that removing this helper made it harder to justify skipping future tests in Windows.

~~Happy to add it back 🙂~~

I remembered the real reason: this helper causes all tests in the file to be skipped, regardless of where it's used.
Of the two tests remaining that would use it, only the yarn pnp one has one test in its file, so using it skips a whole host of other tests that pass on Windows.

Thus, if you take that out of the picture, you're just left with one test that would be using it, and that is unlikely to go away soon from what I understand; I felt that inling it would make it harder to justify skipping tests on Windows in the future.

aight, consider me convinced! 🙂

SimenB · 2020-08-01T13:11:20Z

Windows is falling again 🙂

G-Rath · 2020-08-01T23:32:53Z

The failures are in jest-haste-map, which is interesting as nothing should have changed when I dropped the previous commit 🤔

Looking over the code, theres a few paths I've missed, and at the failures, some of them don't surprise me, i.e

   Map {
    -   "\\project\\fruits\\Pear.js" => 0,
    -   "\\project\\fruits\\another\\Pear.js" => 0,
    +   "/project\\fruits\\Pear.js" => 0,
    +   "/project\\fruits\\another\\Pear.js" => 0,
      }

I originally kept the root slash in, thinking it was part of how haste-map worked, but it seems I was just wrong.
I'll keep chipping away at it and hopefully it'll settle down.

SimenB · 2020-08-02T08:34:51Z

@G-Rath mark as ready wehen it is?

G-Rath · 2020-08-02T08:36:03Z

@SimenB will do - I'm afk right now but when I'm back I'll squash and mark as ready 🎉🎉

SimenB · 2020-08-02T08:38:22Z

We squash merge, so no worries 🙂

SimenB · 2020-08-02T08:40:06Z

Thanks!

github-actions · 2021-05-11T05:07:49Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

facebook-github-bot added the cla signed label Aug 1, 2020

G-Rath commented Aug 1, 2020

View reviewed changes

SimenB approved these changes Aug 1, 2020

View reviewed changes

G-Rath added 4 commits August 2, 2020 18:18

test: allow tests to run on windows

b3754b5

test: remove skipSuiteOnWindows helper

3cd6fa4

tmp: fix haste-map tests more

88cfb17

test: escape Windows path separators in circus mock

9f8b310

SimenB marked this pull request as ready for review August 2, 2020 08:38

SimenB changed the title ~~test: allow tests to run on windows~~ test: allow more tests to run on windows Aug 2, 2020

SimenB merged commit 683b7e6 into jestjs:master Aug 2, 2020

G-Rath deleted the run-tests-on-windows branch August 2, 2020 08:58

G-Rath mentioned this pull request Aug 2, 2020

chore: remove unused errors in catchs, and reject parameters #10354

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: allow more tests to run on windows #10351

test: allow more tests to run on windows #10351

G-Rath commented Aug 1, 2020 •

edited

SimenB commented Aug 1, 2020

G-Rath commented Aug 1, 2020 •

edited

G-Rath Aug 1, 2020 •

edited

SimenB left a comment

SimenB Aug 1, 2020

G-Rath Aug 2, 2020 •

edited

SimenB Aug 1, 2020

G-Rath Aug 1, 2020 •

edited

SimenB Aug 1, 2020

G-Rath Aug 1, 2020 •

edited

SimenB Aug 1, 2020

SimenB commented Aug 1, 2020

G-Rath commented Aug 1, 2020

SimenB commented Aug 2, 2020

G-Rath commented Aug 2, 2020

SimenB commented Aug 2, 2020

SimenB commented Aug 2, 2020

github-actions bot commented May 11, 2021

		it('does not report --notify flag', () => {
		if (process.platform === 'win32') {

test: allow more tests to run on windows #10351

test: allow more tests to run on windows #10351

Conversation

G-Rath commented Aug 1, 2020 • edited

Summary

Test plan

SimenB commented Aug 1, 2020

G-Rath commented Aug 1, 2020 • edited

G-Rath Aug 1, 2020 • edited

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

SimenB Aug 1, 2020

Choose a reason for hiding this comment

G-Rath Aug 2, 2020 • edited

Choose a reason for hiding this comment

SimenB Aug 1, 2020

Choose a reason for hiding this comment

G-Rath Aug 1, 2020 • edited

Choose a reason for hiding this comment

SimenB Aug 1, 2020

Choose a reason for hiding this comment

G-Rath Aug 1, 2020 • edited

Choose a reason for hiding this comment

SimenB Aug 1, 2020

Choose a reason for hiding this comment

SimenB commented Aug 1, 2020

G-Rath commented Aug 1, 2020

SimenB commented Aug 2, 2020

G-Rath commented Aug 2, 2020

SimenB commented Aug 2, 2020

SimenB commented Aug 2, 2020

github-actions bot commented May 11, 2021

G-Rath commented Aug 1, 2020 •

edited

G-Rath commented Aug 1, 2020 •

edited

G-Rath Aug 1, 2020 •

edited

G-Rath Aug 2, 2020 •

edited

G-Rath Aug 1, 2020 •

edited

G-Rath Aug 1, 2020 •

edited