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.

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

[Bug]: running nyc in reportCoverage() fails on Windows with factory file associations settings #459

Closed
pyryk opened this issue May 14, 2024 · 6 comments
Labels
bug Something isn't working needs triage

Comments

@pyryk
Copy link

pyryk commented May 14, 2024

Describe the bug

On Windows, with default Windows settings, generating the coverage report fails due to a Windows Script Host error.

When generating test coverage, the Storybook Test Runner runs the nyc command by executing the nyc.js file directly, rather than explicitly calling node with the file name as an argument. This works on systems that support the shebang directive, but AFAIK, on Windows, there is no shebang directive support. Therefore, Windows can only run the file based on the global file associations settings. Incidentally, the default file association for .js files is not node but the Windows Script Host. Unsurprisingly, the Windows Script Host is not able to interpret or run the nyc.js file.

Since the file associations on Windows are global to the system and prone to change and/or reset over time, I'd argue it would be beneficial to cross-platform compatibility if the test-runner coverage report function would not rely on those settings.

The previous test-runner version (0.17.0) tried to determine the running environment based on the package manager used. Is there a specific reason this approach was removed? As far as I understand, this approach also worked on Windows irrespective of file association settings. Another option would probably be explicitly calling node with the nyc.js file path as an argument (although I'm not sure if this approach would have some other downsides)

To Reproduce

Prerequisites: install node@20 on Windows 10 or 11

  1. Reset Windows file associations to the default values (or set the file association for .js files to the Windows Script Host)
  2. Add a minimal storybook example with a single storybook test
  3. Run npm run test-storybook with coverage enabled
  4. (Expected) a coverage report is generated
    (Actual) A Windows Script Host error popup is shown with message "Microsoft JScript compilation error: invalid character in file nyc.js on line 1, char 1" (paraphrased)

System

Storybook Environment Info:

  System:
    OS: Windows 11 10.0.22631
    CPU: (16) x64 AMD Ryzen 7 PRO 6850U with Radeon Graphics
  Binaries:
    Node: 20.9.0 - C:\Program Files\nodejs\node.EXE
    npm: 10.1.0 - C:\Program Files\nodejs\npm.CMD <----- active
  Browsers:
    Edge: Chromium (123.0.2420.97)
  npmPackages:
    @storybook/addon-a11y: 8.0.10 => 8.0.10
    @storybook/addon-coverage: 1.0.3 => 1.0.3
    @storybook/addon-essentials: 8.0.10 => 8.0.10
    @storybook/addon-interactions: 8.0.10 => 8.0.10
    @storybook/addon-links: 8.0.10 => 8.0.10
    @storybook/addon-mdx-gfm: 8.0.10 => 8.0.10
    @storybook/react: 8.0.10 => 8.0.10
    @storybook/react-vite: 8.0.10 => 8.0.10
    @storybook/test: 8.0.10 => 8.0.10
    @storybook/test-runner: 0.18.0 => 0.18.0
    eslint-plugin-storybook: 0.8.0 => 0.8.0
    storybook: 8.0.10 => 8.0.10

Additional context

No response

@pyryk pyryk added bug Something isn't working needs triage labels May 14, 2024
@yannbf
Copy link
Member

yannbf commented May 16, 2024

Hey @pyryk thanks for reporting this, and for being so detailed about it!

The previous implementation you mentioned actually never worked. As coverage generation runs in the process exit (which should be sync), the previous implementation, given it was asynchronous, would actually be lost and not execute. Your suggestion for running node is pretty good, I'm not sure about potential downsides either.

@yannbf
Copy link
Member

yannbf commented May 16, 2024

@pyryk I made a canary release containing the fix. Would you please try it out?

@storybook/[email protected]

@pyryk
Copy link
Author

pyryk commented May 17, 2024

Thank you for such a quick response! I personally don't have a Windows machine I could test this on, but I asked a colleague to try the canary version. I'll let you know as soon as they've tried it.

@yannbf
Copy link
Member

yannbf commented May 17, 2024

Hey @pyryk this is now released in version 0.18.1, please try it out!

@pyryk
Copy link
Author

pyryk commented May 21, 2024

@yannbf the colleague just confirmed that the updated version works correctly on Windows even with default file associations.

Thank you for taking the time to fix this!

(I suppose it is now OK to close this, please reopen if there is a need for that)

@pyryk pyryk closed this as completed May 21, 2024
@yannbf
Copy link
Member

yannbf commented May 21, 2024

@yannbf the colleague just confirmed that the updated version works correctly on Windows even with default file associations.

Thank you for taking the time to fix this!

(I suppose it is now OK to close this, please reopen if there is a need for that)

Amazing to hear! Thank you so much for reporting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs triage
Projects
None yet
Development

No branches or pull requests

2 participants