testdouble / quibble

Makes it easy to replace require'd dependencies.
94 stars 25 forks source link

fix: enable running on Windows #89

Closed webstech closed 1 year ago

webstech commented 1 year ago

import on Windows requires using the file:// url. This can be used on all platforms for absolute paths.

hackErrorStackToGetCallerFile() returns a pathname which includes a leading '/'. This must be removed on Windows for path.resolve but added back along with converting '\' to '/' for the import path.

This is to support the CI run of semantic-release v20 which uses testdouble. There are a couple of PRs for that project for Windows support as well. The CI runs under Windows with these changes and the other changes.

The tests have been run under Windows wsl. The esm and no-loader-esm tests fail on native Windows due to teenytest issues with Windows. The remaining tests run on Windows.

searls commented 1 year ago

Thank you for submitting this! @giltayar can you take a look?

searls commented 1 year ago

Hi @webstech! TIL that GH Actions supports Windows. Could you rebase main and try to see if you can get this passing before merge?

I opened #90 just to verify if it would or wouldn't pass (it failed)

searls commented 1 year ago

i apparently had the power to rebase it!

webstech commented 1 year ago

try to see if you can get this passing before merge?

If you are willing to make node v14 the minimum engine, then the non-windows compatible if node test/supports-esm.js can be removed from the scripts in package.json. Node v14 is the oldest LTS version and had some security fixes last year that should have got people moving on from node v12.

I ran with this in the package.json:

"test:esm": "node --loader=quibble ./node_modules/teenytest/bin/teenytest.js ./test/esm-lib/*.test.{mjs,js}"

However, as I said earlier, teenytest has Windows issues. I do remember the stopping point was in lib\prepare\modules\load.js where require is using a fully qualified name. This does not appear to work in node under Windows - the doc mentions using search rules. I did make it work using relative paths so there may be a node issue here.

searls commented 1 year ago

If you are willing to make node v14 the minimum engine, then the non-windows compatible if node test/supports-esm.js can be removed from the scripts in package.json. Node v14 is the oldest LTS version and had some security fixes last year that should have got people moving on from node v12.

Yeah, totally ok with that. That's what the matrix already does

However, as I said https://github.com/testdouble/quibble/pull/89#issue-1551715889, teenytest has Windows issues. I do remember the stopping point was in lib\prepare\modules\load.js where require is using a fully qualified name. This does not appear to work in node under Windows - the doc mentions using search rules. I did make it work using relative paths so there may be a node issue here.

Sorry, I totally missed that. Hmm

webstech commented 1 year ago

I went back to specifying relative paths in teenytest\lib\prepare\modules\load.js to verify windows was working and this appears to need more work. Setting to draft. Sorry about that.

Edit: The original patch works with semantic-release on Windows. The CI test has exposed other issues. e.g. quibble.esmImportWithPath was not tested.

webstech commented 1 year ago

Sorry, I totally missed that. Hmm

Initial testing shows positive results changing this line to:

var testPath = path.relative(__dirname, file)

may resolve the teenytest issue by 'require'ing relative to the loading file.

webstech commented 1 year ago

If you are willing to make node v14 the minimum engine, then the non-windows compatible if node test/supports-esm.js can be removed from the scripts in package.json. Node v14 is the oldest LTS version and had some security fixes last year that should have got people moving on from node v12.

Yeah, totally ok with that. That's what the matrix already does

PR #92 has been opened for this, It is competing with PR #91 but rebasing should not be an issue.

webstech commented 1 year ago

This has been tested with PR #91. It is dependent on teenytest PR #75 for running on Windows.

Windows testing was done using wsl, bash and cmd.

Removing draft status.

searls commented 1 year ago

Look at that!

Screenshot 2023-02-12 at 9 34 33 AM

Everything merged and updated, and we're running on windows! Published as v0.6.16