Build: Run unit tests on more node versions, mac, and windows #16744

IanVS · 2021-11-22T02:23:14Z

Issue: N/A

What I did

In #16723 (review), it was requested to run tests on Windows. It surprised me a bit that unit tests were not already automatically running on windows for every PR. So, this PR adds windows to the unit tests in the github action. I also took things a step further and added more versions of node.js, as well as MacOS.

According to #16664, we can expect the windows tests to fail. And I'm not sure if there's been discussion previously on adding windows tests or not, but I figured I'd throw this up and see what happens. :)

How to test

Is this testable with Jest or Chromatic screenshots?
Does this need a new example in the kitchen sink apps?
Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

nx-cloud · 2021-11-22T02:23:22Z

Nx Cloud Report

CI ran the following commands for commit dacc755. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status	Command
	`nx run-many --target=prepare --all --parallel --max-parallel=15`

Sent with 💌 from NxCloud.

nx-cloud · 2021-11-22T02:23:22Z

Nx Cloud Report

We didn't find any information for the current pull request with the commit 2548872.
You might need to set the 'NX_BRANCH' environment variable in your CI pipeline.

Check the Nx Cloud Github Integration documentation for more information.

Sent with 💌 from NxCloud.

IanVS · 2021-11-22T04:42:42Z

As expected, Windows tests are failing. I'm not sure how we'd even get them passing, TBH.

Interestingly, Node 17 is also failing, due to something in webpack 5, it seems:

  this[kHandle] = new _Hash(algorithm, xofLen);
                  ^

Error: error:0308010C:digital envelope routines::unsupported
    at new Hash (node:internal/crypto/hash:67:19)
    at Proxy.createHash (node:crypto:130:10)
    at module.exports (/home/runner/work/storybook/storybook/node_modules/webpack/lib/util/createHash.js:135:53)
    at NormalModule._initBuildHash (/home/runner/work/storybook/storybook/node_modules/webpack/lib/NormalModule.js:417:16)
    at handleParseError (/home/runner/work/storybook/storybook/node_modules/webpack/lib/NormalModule.js:471:10)
    at /home/runner/work/storybook/storybook/node_modules/webpack/lib/NormalModule.js:503:5
    at /home/runner/work/storybook/storybook/node_modules/webpack/lib/NormalModule.js:358:12
    at /home/runner/work/storybook/storybook/node_modules/loader-runner/lib/LoaderRunner.js:373:3
    at iterateNormalLoaders (/home/runner/work/storybook/storybook/node_modules/loader-runner/lib/LoaderRunner.js:214:10)
    at Array.<anonymous> (/home/runner/work/storybook/storybook/node_modules/loader-runner/lib/LoaderRunner.js:205:4)
    at Storage.finished (/home/runner/work/storybook/storybook/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:55:16)
    at /home/runner/work/storybook/storybook/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:91:9
    at /home/runner/work/storybook/storybook/node_modules/graceful-fs/graceful-fs.js:123:16
    at FSReqCallback.readFileAfterClose [as oncomplete] (node:internal/fs/read_file_context:68:3) {
  opensslErrorStack: [ 'error:03000086:digital envelope routines::initialization error' ],
  library: 'digital envelope routines',
  reason: 'unsupported',
  code: 'ERR_OSSL_EVP_UNSUPPORTED'
}

Node.js v17.1.0

Ah, yes this Node 17 problem has been raised in an issue: #16555

IanVS · 2021-11-22T15:42:36Z

One approach might be to use https://www.npmjs.com/package/jest-os-detection to write linux and windows specific tests that only run on particular operating systems.

But, I'd like to know if the team thinks this is worth exploring, before I put any time into it. Also, if anyone else already has a good windows environment set up, I'd be happy to turn this effort over to them. :)

ndelangen · 2021-11-22T16:14:11Z

Looks like (at least part of) the reason why tests are failing on windows is because of line-ending issues:

IanVS · 2021-11-22T16:16:02Z

@ndelangen exactly. Line endings and path separator differences (/ vs \) account for many of the failures, I believe.

IanVS · 2021-11-23T22:15:53Z

OK, it'll take a while for the CI to run, but I think I've at least gotten the snapshots passing on both windows and linux. I'll take a look at the unit tests next.

IanVS · 2021-11-24T17:38:11Z

@ndelangen is it possible that this is a legit failure on windows?

loadPreset › should prepend framework field to list of presets

    expect(received).toMatchInlineSnapshot(snapshot)

    Snapshot name: `loadPreset should prepend framework field to list of presets 1`

    - Snapshot  - 5
    + Received  + 0

    @@ -8,15 +8,10 @@
          "name": "@storybook/preset-typescript",
          "options": Object {},
          "preset": Object {},
        },
        Object {
    -     "name": "@storybook/addon-docs/preset",
    -     "options": Object {},
    -     "preset": Object {},
    -   },
    -   Object {
          "name": Object {
            "addons": Array [
              "@storybook/addon-docs",
            ],
            "framework": "@storybook/react",

      436 |       {}
      437 |     );
    > 438 |     expect(loaded).toMatchInlineSnapshot(`
          |                    ^
      439 |       Array [
      440 |         Object {
      441 |           "name": "@storybook/react",

      at Object.<anonymous> (lib/core-common/src/presets.test.ts:438:20)

ndelangen · 2021-11-25T08:16:04Z

That doesn't look good indeed. @IanVS

Marklb · 2021-12-03T23:58:50Z

I have taken a look at the Angular tests on Windows and usually end up just running the specific tests that I know my change could affect and just let the rest run in CI, or a linux VM if I am fairly sure I will need to rerun the tests multiple times. It would be nice to not have to do that. I am not the most experienced with unit tests, but I can try to help get the Angular ones passing, at least.

One tricky thing I have run into with tests on Windows is the different valid path formats, depending on what will be processing them. I was trying to fix one last night and I think it was a Typescript Webpack plugin had a path in the format E:\Users\Docs\Project then an Angular Webpack plugin had a path in the format E:\\Users\\Docs\\Project then something else had a path in the format E:/Users/Docs/Project.

Marklb · 2021-12-04T12:23:39Z

Well, I took a look at the Angular errors, again and even though I would like it to be more obvious which path formatting should be used, they weren't as far off from fixing as I remembered from last time I looked. I haven't tested them outside of windows yet, but I can do that then submit them or you can just pick them out of my fork if it's useful. It isn't much: Marklb@e432ea0

IanVS · 2021-12-12T03:03:10Z

OK, I've made some updates and verified that the unit test matrix jobs do not run on a PR unless the ci:matrix label is added, and that they start as soon as the label is added. The tests should also run on merges to next. I've just rebased on next, and believe this is ready for review now.

IanVS · 2021-12-12T16:24:05Z

OK, with the latest commit I've made sure the tests continue to run when new commits are pushed as long as that label still is present.

kylegach · 2021-12-13T21:51:47Z

lib/core-server/src/core-presets.test.ts

 // this only applies to this file
 jest.setTimeout(10000);

+// FIXME: this doesn't work


Comments on this FIXME, @IanVS?

I found that the stories.json file was still being created. Also, when I turned on logging, I was seeing messages about this preset being invalid. I think maybe it's not in the correct format for a preset, perhaps?

kylegach

Thank you for this very thoughtful PR, @IanVS. Future contributors on Windows should have a much nicer experience.

These changes look good to me, but people more familiar with Windows dev and/or @shilman, @ndelangen, @tmeasday should provide final approval.

shilman

@IanVS This is amazing work. Thanks so much for putting the time in, and also for your patience on the merge. I'm guessing we'll need to tweak a bit once we run this in practice, but I'll coordinate that with you when the time comes. In the meantime, merging! 💯

IanVS · 2021-12-15T03:13:18Z

Thanks @shilman. And just to close the loop on one point, it does look like the unit tests are running against the next branch, as intended:

ndelangen · 2022-03-03T12:47:41Z

I think running the unit tests on multiple platforms is great, but taking different snapshot on different platforms, will make things more difficult.

If someone needs to update the snapshots, they need access to both platforms, which is not something everyone has (me included, I do not have access to any windows platform device)

IanVS · 2022-03-03T14:41:04Z

That's a good point. I was using a virtual machine on my mac when I was working on this PR, but that's a pain. I could see a few different possible solutions.

Find a way to accept snapshot changes within CI (maybe this is possible?).
Let them fail until someone with windows can update them.
Remove the snapshots, replace with more explicit tests.
Remove windows tests in CI.

I'd love if we could find an option that's not #4, since I think it's good to have visibility into the windows experience. But, I'll understand if that's the way y'all want to go.

tmeasday · 2022-03-04T18:55:08Z

Find a way to accept snapshot changes within CI (maybe this is possible?).

I'm not necessarily advocating for this but with Circle it is possible to create downloadable artifacts from a build. What we did in a similar situation in the Chromatic monorepo is do the snapshot tests in CI in two phases:

Run, record exit code.
If 1. failed, run a second time with --updateSnapshot --onlyFailures, place snapshots in artifact location
Return exit code from 1.

It's a bit clunky but it does mean as a dev you can download the snapshots generated in CI in 2.

IanVS added other windows labels Nov 22, 2021

IanVS mentioned this pull request Nov 22, 2021

Core: Add ./ to start of hidden file & folder paths #16723

Merged

3 tasks

MichaelArestad assigned ndelangen Nov 22, 2021

IanVS force-pushed the windows-tests branch 5 times, most recently from b63347d to 2412607 Compare November 23, 2021 22:13

IanVS force-pushed the windows-tests branch 2 times, most recently from 032a0db to 1faf93d Compare November 24, 2021 01:44

IanVS mentioned this pull request Nov 24, 2021

Sometimes uses old title when running on mac igor-dv/jest-specific-snapshot#39

Open

IanVS force-pushed the windows-tests branch from a9b28c9 to 56b5948 Compare November 24, 2021 16:24

shilman added maintenance User-facing maintenance tasks and removed other labels Nov 24, 2021

shilman changed the title ~~Run unit tests on more node versions, mac, and windows~~ Build: Run unit tests on more node versions, mac, and windows Nov 24, 2021

MichaelArestad assigned kylegach Nov 29, 2021

IanVS force-pushed the windows-tests branch 2 times, most recently from 8a5b383 to 1dad0ee Compare December 4, 2021 23:32

IanVS removed the ci:matrix label Dec 12, 2021

IanVS force-pushed the windows-tests branch from a4911b9 to cadbc66 Compare December 12, 2021 02:49

IanVS added the ci:matrix label Dec 12, 2021

IanVS added 13 commits December 11, 2021 21:10

Run unit tests on more node versions, mac, and windows

d5e9404

Split some snapshots by windows/posix

dcfdeb8

[wip] Fix unit tests on windows

3d64cda

Avoid concurrent test failures

79cc0ab

Fix angular cli windows tests

595ce8a

Fix windows tests for normalizeStoryPath

e65bd0e

Fix windows tests failing from main.js mock

24a2535

Increase timeouts for windows

23383b7

Normalize windows paths in invalidation

4fca87b

Do not use join in resolveFrom

8d13cfe

Remove node 17 for now

c8e0eab

Fix watch for windows directories

2f781b8

Limit execution of matrix CI in PRs

2497080

IanVS force-pushed the windows-tests branch from cadbc66 to 2497080 Compare December 12, 2021 05:11

Include pull request with existing label

dacc755

IanVS force-pushed the windows-tests branch from 40459d0 to dacc755 Compare December 12, 2021 16:23

kylegach reviewed Dec 13, 2021

View reviewed changes

shilman approved these changes Dec 15, 2021

View reviewed changes

shilman merged commit e7b8c49 into next Dec 15, 2021

shilman deleted the windows-tests branch December 15, 2021 02:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build: Run unit tests on more node versions, mac, and windows #16744

Build: Run unit tests on more node versions, mac, and windows #16744

IanVS commented Nov 22, 2021

nx-cloud bot commented Nov 22, 2021 •

edited

Loading

nx-cloud bot commented Nov 22, 2021

IanVS commented Nov 22, 2021 •

edited

Loading

IanVS commented Nov 22, 2021 •

edited

Loading

ndelangen commented Nov 22, 2021 •

edited

Loading

IanVS commented Nov 22, 2021

IanVS commented Nov 23, 2021

IanVS commented Nov 24, 2021

ndelangen commented Nov 25, 2021

Marklb commented Dec 3, 2021

Marklb commented Dec 4, 2021

IanVS commented Dec 12, 2021

IanVS commented Dec 12, 2021

kylegach Dec 13, 2021

IanVS Dec 14, 2021

kylegach left a comment

shilman left a comment

IanVS commented Dec 15, 2021

ndelangen commented Mar 3, 2022

IanVS commented Mar 3, 2022

tmeasday commented Mar 4, 2022

Build: Run unit tests on more node versions, mac, and windows #16744

Build: Run unit tests on more node versions, mac, and windows #16744

Conversation

IanVS commented Nov 22, 2021

What I did

How to test

nx-cloud bot commented Nov 22, 2021 • edited Loading

Nx Cloud Report

nx-cloud bot commented Nov 22, 2021

Nx Cloud Report

IanVS commented Nov 22, 2021 • edited Loading

IanVS commented Nov 22, 2021 • edited Loading

ndelangen commented Nov 22, 2021 • edited Loading

IanVS commented Nov 22, 2021

IanVS commented Nov 23, 2021

IanVS commented Nov 24, 2021

ndelangen commented Nov 25, 2021

Marklb commented Dec 3, 2021

Marklb commented Dec 4, 2021

IanVS commented Dec 12, 2021

IanVS commented Dec 12, 2021

kylegach Dec 13, 2021

Choose a reason for hiding this comment

IanVS Dec 14, 2021

Choose a reason for hiding this comment

kylegach left a comment

Choose a reason for hiding this comment

shilman left a comment

Choose a reason for hiding this comment

IanVS commented Dec 15, 2021

ndelangen commented Mar 3, 2022

IanVS commented Mar 3, 2022

tmeasday commented Mar 4, 2022

nx-cloud bot commented Nov 22, 2021 •

edited

Loading

IanVS commented Nov 22, 2021 •

edited

Loading

IanVS commented Nov 22, 2021 •

edited

Loading

ndelangen commented Nov 22, 2021 •

edited

Loading