chore: support tests on win32

[mbtools]

The pacote tests don't run on Windows (surprisingly). Ergo, Windows is not included in ci.

This PR ports the test cases to win32... almost 😃

• Consolidate cleaning of snapshots into helpers/clean-snapshots.js
• Consistently use {VAR} instead of ${VAR} as placeholders
• Adjust paths to Windows format were necessary
• Adapt to slight differences in error handling on Windows
• Consolidate chmode for script.js into helpers/script-mode.js (see below)

Tests

node: 20.15.1 npm: 10.8.2 win: 10.0.19045.0

Just one failure in git.js. The two skips are the helpers (see above).

Coverage

Almost complete

Open Issues

1. mode for script.js

On my Windows 10, script.js is set to 0o666 which stands for standard r/w access but is not executable. This difference is also responsible for the missing line in coverage of file.js. Should this be mocked to match Unix 0o111? How?

2. avoiding "spawn npm ENOENT"

I want to avoid this change in lib\util\npm.js (the only one outside of /test/). Is there a better way? How do other npm modules handle this on Windows?

3. git tests

There's one case that I can't solve in git.js. Any ideas?

test/git.js 2> Error: EBUSY: resource busy or locked, rmdir 'C:\Github\~npm\pacote\test\tap-testdir-git'
test/git.js 2> 
test/git.js 2> {
test/git.js 2>   name: 'TAP',
test/git.js 2>   autoend: true,
test/git.js 2>   errno: -4082,
test/git.js 2>   code: 'EBUSY',
test/git.js 2>   syscall: 'rmdir',
test/git.js 2>   path: 'C:\\Github\\~npm\\pacote\\test\\tap-testdir-git',
test/git.js 2>   tapCaught: 'unhandledRejection',
test/git.js 2>   test: 'TAP'
test/git.js 2> }
test/git.js 2> 
test/git.js 2>   #  C:\Windows\system32\cmd.exe [1344]:  at c:\ws\src\api\callback.cc:145
test/git.js 2>   #  Assertion failed: (env_->execution_async_id()) == (0)
test/git.js 2> 
test/git.js 2> ----- Native stack trace -----
test/git.js 2> 
test/git.js 2>  1: 00007FF67816EEDB node::SetCppgcReference+18123
test/git.js 2>  2: 00007FF6780DECF1 DSA_meth_get_flags+81937
test/git.js 2>  3: 00007FF6781A5E00 node::CallbackScope::~CallbackScope+848
test/git.js 2>  4: 00007FF6781A5B1E node::CallbackScope::~CallbackScope+110
test/git.js 2>  5: 00007FF678163F20 node_api_throw_syntax_error+170432
test/git.js 2>  6: 00007FF67814C11D node_api_throw_syntax_error+72637
test/git.js 2>  7: 00007FF6781C7A68 uv_pipe_pending_type+856
test/git.js 2>  8: 00007FF6781D452D uv_run+1021
test/git.js 2>  9: 00007FF6781A53B5 node::SpinEventLoop+405
test/git.js 2> 10: 00007FF67808CD52 X509_STORE_get_cleanup+154850
test/git.js 2> 11: 00007FF6781255BD node::Start+4909
test/git.js 2> 12: 00007FF6781242C0 node::Start+48
test/git.js 2> 13: 00007FF677EDD74C AES_cbc_encrypt+150908
test/git.js 2> 14: 00007FF67936715C inflateValidate+19196
test/git.js 2> 15: 00007FFC86097374 BaseThreadInitThunk+20
test/git.js 2> 16: 00007FFC8765CC91 RtlUserThreadStart+33
​ FAIL ​ test/git.js 70 OK 1m
  command: C:\Program Files\nodejs\node.exe
  args:
    - test/git.js
  exitCode: 134
  signal: null

[mbtools]

There were significant changes between 18.0.6 and 19.0.1 which I did not have on my system (I should have rebased before).

Moving to draft until I resolve those.

[wraithgar]

If you remove this line and run npm run template-oss-apply that will add windows back into the testing matrix.

Fun fact: that flag was first added to template-oss because of this repo.

[wraithgar]

This is quite a big undertaking. Even if we only get partway there it'll be worth landing changes that get us closer. If we can't get tests 100% working in windows we won't be able to enable them in CI, but it'll still be worth "moving the needle" a bit to get closer.

[wraithgar]

The EBUSY error is a known race condition with tap tearing down its fixtures iirc. I don't know if we ever found a consistent workaround but iirc you can try to wrap the root-level tap.testdir (which is what's making that fixture) in a t.test. What that really means is wrapping everything from

const me = t.testdir({
  repo: {},
  cache: {},
})

to the end of the test file in:

t.test('git', async t => {
// await existing tests
})`

[wraithgar]

The missing coverage in file.js is something we'll need to dig into a little deeper. If there is missing coverage in windows that means we are likely missing something, and the code needs to be cleaned up in some way. Can you explain this one in more depth? What code path is missed? What ends up happening in windows when that function runs?

[wraithgar]

This is a decent way to fix the EBUSY errors in Windows when using a tap that uses libtap https://github.com/isaacs/rimraf/blob/main/libtap-settings.js

Getting pacote onto the latest tap is one of our follow-up tasks for npm 11.

[mbtools]

Thanks for all the feedback. Looks like rimraf works better on Win than fs.rmSync. Issac mentioned it somewhere else, too.

It seems promise-spawn has an issue if the shell contains spaces for example in C:\Program Files\nodejs\node.exe npm .... Some quotes are missing. I'm working on it.

If this PR becomes too much, I will split it up.

[wraithgar]

Just a heads up that we're landing a breaking change soon. So far it doesn't look like this PR will merge conflict with it, but if it does and you want help sorting it out let us know.

[mbtools]

This was driving me nuts but here's an interesting find: tap on Windows does not use node:spawn directly but cross-spawn

https://github.com/tapjs/foreground-child/blob/f2e72ffc2a8077d26e1cacfc6944f16befaeafc8/src/index.ts#L15

AFAICT, tests based on npm:promise-spawn running on Windows don't execute the same code as when run live. 🤔

I think promise-spawn needs a good work-over (I agree with https://github.com/npm/promise-spawn/issues/73 very much). spawnWithShell should be removed and it should use cross-spawn the same way as tap (i.e. on win32). This would probably be breaking. Maybe pacote shouldn't use promise-spawn at all.

I skipped the git.js tests on win32. The other tests were completed successfully, with coverage of 86%. That's as good as it gets.

[wraithgar]

Hey @mbtools we haven't forgotten about you. We're focusing on a release of npm 10 to address an audit warning, and npm 11 too.

[mbtools]

thanks for the note. no worry.

[wraithgar]

Are you comfortable with git rebase? If so can you isolate the addition of rimraf into its own chore: commit, and all the other changes into a second chore: commit? If not that's no problem we can do that, but git will then give us credit for the commits.