More watch test stabilization

[lukastaegert]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:

Description

Most notably watch-config-early-update still fails sometimes.

[github-actions]

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:
https://rollupjs.org/repl/?pr=4339

[codecov]

Codecov Report

Merging #4339 (e27e118) into master (205e861) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            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     

Impacted Files	Coverage Δ
cli/run/watch-cli.ts	90.54% <0.00%> (-1.36%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 205e861...e27e118. Read the comment docs.

[kzc]

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.
         );
       });
       `

timeout	node-v12 percent failure	node-v16 percent failure
112	100	100
113	90	100
114	80	70
115	0	10
116	0	0

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
         );
       });
       `

[kzc]

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?

[lukastaegert]

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.

[lukastaegert]

Ok, went with the bigger timeout after all. Seems fairly stable for now.

[kzc]

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);
 }