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
More watch test stabilization #4339
Conversation
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via npm install rollup/rollup#more-watch-test-stabilization or load it into the REPL: |
Codecov Report
@@ Coverage Diff @@
## master #4339 +/- ##
==========================================
- Coverage 98.44% 98.43% -0.02%
==========================================
Files 206 206
Lines 7350 7350
Branches 2088 2088
==========================================
- Hits 7236 7235 -1
Misses 55 55
- Partials 59 60 +1
Continue to review full report at Codecov.
|
95ee894
to
4d445ee
Compare
On my Mac I can reproduce the error in question by changing this timeout: --- a/test/cli/samples/watch/watch-config-early-update/_config.js
+++ b/test/cli/samples/watch/watch-config-early-update/_config.js
@@ -5,6 +5,7 @@ const { atomicWriteFileSync } = require('../../../../utils');
let configFile;
module.exports = {
+ repeat: 10,
description: 'immediately reloads the config file if a change happens while it is parsed',
command: 'rollup -cw',
before() {
@@ -24,7 +25,7 @@ module.exports = {
format: 'es'
}
}),
- 1000
+ 114 // <------ 70% failure with 114ms. See chart below.
);
});
`
This ought to be enough to sway the race condition towards reliability on github CI: --- a/test/cli/samples/watch/watch-config-early-update/_config.js
+++ b/test/cli/samples/watch/watch-config-early-update/_config.js
@@ -24,7 +24,7 @@ module.exports = {
format: 'es'
}
}),
- 1000
+ 3000
);
});
` |
009ced5
to
d995cdf
Compare
Isn't having a watcher of the config file followed by a timeout essentially a more elaborate version of the original test timeout race condition? |
It is, if it works, it is just much faster on a local machine while adapting to slow CI machines. The reason that a timeout is still necessary confuses me too, though. And maybe it is just stupid. Update: I think we need the timeout because chokidar may be too slow to report the update to Rollup due to some debounce timeout. Thus without the timeout, it still bundles the first config and then bundles the second config after that. |
Ok, went with the bigger timeout after all. Seems fairly stable for now. |
I think the simple timeout increase is best. The last few macOS releases reportedly have had markedly higher filesystem latency compared to the OS X releases several years ago. Some users have also reported peculiar random pauses in filesystem use. This could explain why the macOS tests required longer timeouts. Regarding the watch delays needed after a supposed synchronous file write, perhaps the reason is that the file on the filesystem needs to be properly flushed to the physical media. This patch might aid that cause: --- a/test/utils.js
+++ b/test/utils.js
@@ -16,6 +16,7 @@ exports.assertDirectoriesAreEqual = assertDirectoriesAreEqual;
exports.assertFilesAreEqual = assertFilesAreEqual;
exports.assertIncludes = assertIncludes;
exports.atomicWriteFileSync = atomicWriteFileSync;
+exports.syncFile = syncFile;
function normaliseError(error) {
delete error.stack;
@@ -229,5 +230,13 @@ function assertIncludes(actual, expected) {
function atomicWriteFileSync(filePath, contents) {
const stagingPath = filePath + '_';
fs.writeFileSync(stagingPath, contents);
+ syncFile(stagingPath);
fs.renameSync(stagingPath, filePath);
+ syncFile(filePath);
+}
+
+function syncFile(filepath) {
+ let fd = fs.openSync(filepath, 'r+');
+ fs.fsyncSync(fd);
+ fs.closeSync(fd);
} |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
Most notably watch-config-early-update still fails sometimes.