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

Tests fails on windows #1

Open
ghost opened this issue Jul 22, 2016 · 5 comments
Open

Tests fails on windows #1

ghost opened this issue Jul 22, 2016 · 5 comments
Labels

Comments

@ghost
Copy link

ghost commented Jul 22, 2016

Great work, except 7 tests fails on Windows.

Here is one error message:

  Stack:
    Error: Expected spy 2 to have been called with [ 'Hello World1', '1', 'test|src|simple.js' ] but actual calls were [ 'Hello World1', '1', 'test\src\simple.js' ].
        at Object.it (....nutra.js:540:26)
  Message:
    Expected spy 3 to have been called with [ 'Hello World2', '2', 'test|

Also for Windows, are the path "transpiled" from unix file path style to Windows file path style?

@m-a-r-c-e-l-i-n-o
Copy link
Owner

m-a-r-c-e-l-i-n-o commented Jul 22, 2016

@trixlerjs Thanks for the inquiry, it's still in beta (will be off beta in v1), so bear with me. Any chance you could post all the 7 failed tests you see on Windows? Running on a Linux environment here, Windows hasn't been a priority.

Also for Windows, are the path "transpiled" from unix file path style to Windows file path style?

Usually, but in some cases that is handled explicitly, and I must not have accounted for something.

@ghost
Copy link
Author

ghost commented Jul 22, 2016

Hey. I noticed that for a few of the failing tests, there are issues with Unix vs Windows file path. Study this error: `Error: Expected spy 2 to have been called with [ 'Hello World1', '1', 'test|src|simple.js' ] but actual calls were [ 'Hello World1', '1', 'test\src\simple.js' ].

The 7 failing test is this

Nutra private.getSystemConstants() should return a system constants object with default properties

This tests fail because C:/ expected to be equal to C:\

Again a file path issue.

Failing tests:

  • Nutra private.runPreprocessorOnFileLoadHooks() should trigger hooks with "source", "filename", and "key" arguments
  • Nutra private.runPreprocessorOnFileLoadHooks() should trigger hooks with the modified source from a previous hook
  • Nutra private.runPreprocessorOnFileLoadHooks() should trigger hooks with the modified filename and source from a previous hook
  • Nutra private.normalizeFiles() should return list of file paths with custom base path
  • Nutra private.expandFiles() should return list with full file paths with no duplicates
    • Nutra private.getPreprocessorFilters() should return an object of plugin names with their absolute patterns

@ghost
Copy link
Author

ghost commented Jul 22, 2016

If you add AppVeyor to the repo you will allways know if Windows fails

@moshmage
Copy link

moshmage commented Nov 2, 2016

as a fast patch-it-up, I added

// node_modules/nutra/dist/nutra.js:153 @ getSystemConstants(opts)
tmpDirectory: opts.tmpDirectory || this.makeUniqueTmpFolder(),

made tmpDirectory: '/nutra-tmp/' (gitbash for windows understands that much) and then I added a replace to the key replacing | with / (why on earth does the filename come with |? - I couldn't find the answer in my fast screening ;)) like so:

// node_modules/nutra-coverage/dist/preprocessor.js:33 @ const preprocessor
        const tmpFilename = _path2.default.join(system.tmpDirectory, 'coverage', key.replace(/\|/g,'/'));
        const sourceFilename = _path2.default.join(process.cwd(), key.replace(/\|/g, '/'));

... It's not pretty, and I might go around and do it the proper way - but I was able to get that sweet coverage working :D

@m-a-r-c-e-l-i-n-o
Copy link
Owner

m-a-r-c-e-l-i-n-o commented Nov 3, 2016

@moshmage I'm glad you got it working! The file paths have the | because, ironically, I wanted to safe guard against long path issues on Windows for when I got around to officially supporting it, but clearly I didn't pick a very good delimiter, hehe. The choice was made based on aesthetics, since under some conditions the temp filename might appear on the coverage report and | looked cleaner than a bunch of % characters. So property escaping the filename here, which could be done through here, would likely be a more sustainable solution.

With the fast patch you made, you might actually run into filename-too-long issues at some point (max 255-260 characters — if you are on Windows 10 you can change the limit). But I guess you are good for now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants