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

Fix comparison tests under WSL #1109

Merged
merged 11 commits into from May 27, 2020
Merged

Fix comparison tests under WSL #1109

merged 11 commits into from May 27, 2020

Conversation

appzuka
Copy link
Member

@appzuka appzuka commented May 14, 2020

Under WSL2 (Windows Subsystem for Linux) the comparison tests were sometimes failing. The error showed that in patch0 the app.ts file was being rebuilt.

In watchpack, any file which is newer than the last compilation will be recompiled. Although app.ts was not changed between the first compilation and patch0, watchpack is conservative and if the file's timestamp is older than the last compilation time by less than the file system accuracy, it will assume that the file may be new and will include it for compilation. Although the bundle output is the same, the output includes a [built] tag, which causes a comparison error.

On my system I found that the file system accuracy (FS_ACCURACY in watchpack) for Windows 10 was 10ms but for WSL2 it was 100ms. This explains why the tests failed under WSL but not native Windows.

A delay has already been included in copyPatchOrEndTest with the comment that without the delay results can be inconsistent. copyPatchOrEndTest only affects later tests so a 200ms delay has been added between copying the test files and starting webpack. With this change all tests pass under Windows and WSL.

@johnnyreilly
Copy link
Member

johnnyreilly commented May 24, 2020

Thanks for having a crack at this @appzuka! It looks like the tests are failing though? Also there's some conflicts...

createWebpackConfig(paths, options, nonWatchNonCompositePath !== testPath)
).watch({ aggregateTimeout: 1500 }, createWebpackWatchHandler(done, paths, testState, options, test));

// Need to wait > FS_ACCURACY as defined in watchpack.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a link to the details in watchpack that discuss this? Would be super useful for anyone else taking a look at this.

@appzuka
Copy link
Member Author

appzuka commented May 24, 2020

In watchpack FS_ACCURACY is initially defined as 1000ms:

https://github.com/webpack/watchpack/blob/f4f8d9b86c28b1c4547ff100f332dc65344f3eaf/lib/DirectoryWatcher.js#L13

Through ensureFsAccuracy this is reduced to 1, 10 or 100ms by testing the resolution of time in the OS being used:

https://github.com/webpack/watchpack/blob/f4f8d9b86c28b1c4547ff100f332dc65344f3eaf/lib/DirectoryWatcher.js#L757-L762

On my system I found that under Windows10 ensureFsAccuracy set FS_ACCURACY to 10ms but under WSL2 it was 100ms.

The code in watchpack defines a safeTime. If the file's modification date is before this time then the file is deemed to be unchanged and will not be included in new builds. To be safe, files which were changed less than FS_ACCURACY after the safetime are included. I believe watchpack is being conservative and including files which potentially changed, even if they did not. Although this does not affect the final build output, the output text includes "[built]" to show that that file was recompiled. This causes a comparison error on WSL because the file was rebuilt under WSL but not under Windows. The errors are not consistent and could be different under different operating systems and build hosts.

In normal use this way of defining changed files would not cause a problem - users are not going to save a file within 100ms of starting a webpack with --watch. But for the automated tests this can cause a problem because phases of tests can occur very quickly after each other.

In create-and-execute-tests there is a comment that tests are flaky if a patch is applied right away and a 1000ms delay is inserted:

// can get inconsistent results if copying right away

I believe the tests are flaky without this delay for the same reason. However, this delay only fixes the patches. We also need to wait between copying the initial test files and starting the watch. This is the patch I have included.

I used a delay of 200ms. In an ideal world it would be nice to use the same code to measure FS_ACCURACY in case it was set at 1000ms. Or a delay of 1000ms could be used anyway, at the expense of slowing all comparison tests. I judged that 200ms is likely to be safe and not introduce too much slowdown.

@johnnyreilly
Copy link
Member

johnnyreilly commented May 24, 2020

This is really cool! Could you put a comment in the code that briefly explains this (and maybe links back to this PR)?

Side question: how have you found WSL 2? I haven't tried it out yet but I intend to!

@appzuka
Copy link
Member Author

appzuka commented May 24, 2020

I have added a couple of comments linking to the PR.

For me, WSL is what made Windows into a viable development platform and WSL2 has some significant improvements. I used to use a Hackintosh but found it was taking a lot of time to keep the system upgraded and stable. Windows 10 is nice for most applications but it was a pain not having a unix-like terminal. Using a VM was not the same as you could not use Windows native editors directly.

WSL gives you a unix-like terminal which works just like your production environment. You can edit directly in the WSL folder using Windows native VSCode and the Remote-WSL extension.

WSL1 had some drawbacks, especially that copying large numbers of files (such as on yarn install) was very slow. In WSL2 this is improved and the file systems of WSL and Windows are better integrated. From the WSL2 command line you can type 'explorer.exe .' and a Windows File Explorer window opens up in your WSL folder.

The new Windows Terminal app lets you open WSL, CMD and Powershell terminals in separate tabs. And Docker now lets you run docker from within WSL. It is pretty seamlessly integrated and WSL2 is my main development environment now.

@johnnyreilly
Copy link
Member

WSL1 had some drawbacks, especially that copying large numbers of files (such as on yarn install) was very slow. In WSL2 this is improved and the file systems of WSL and Windows are better integrated. From the WSL2 command line you can type 'explorer.exe .' and a Windows File Explorer window opens up in your WSL folder.

The new Windows Terminal app lets you open WSL, CMD and Powershell terminals in separate tabs. And Docker now lets you run docker from within WSL. It is pretty seamlessly integrated and WSL2 is my main development environment now.

This is so cool! At home my main machine runs Ubuntu which I really like. Work is Windows 10. I like it well enough but I'd like to unify my home and work development workflows. WSL 1 is cool but (as you say) pretty slow. I'd heard speed was much better with WSL 2 so I've high hopes for it!

@johnnyreilly
Copy link
Member

Seriously how many days can ci take?!

@johnnyreilly
Copy link
Member

Thanks @appzuka!

@johnnyreilly johnnyreilly merged commit 8b7c8ed into TypeStrong:master May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants