Stabilize watch tests

[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:
#4300

Description

This attempts to follow ideas in #4300 (comment) and others to stabilize the watch tests especially on MacOS.

[github-actions]

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#stabilize-watch-tests

or load it into the REPL:
https://rollupjs.org/repl/?pr=4318

[codecov]

Codecov Report

Merging #4318 (9c008de) into master (2a899d5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4318   +/-   ##
=======================================
  Coverage   98.44%   98.44%           
=======================================
  Files         205      205           
  Lines        7314     7314           
  Branches     2084     2084           
=======================================
  Hits         7200     7200           
  Misses         55       55           
  Partials       59       59           

---

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 2a899d5...9c008de. Read the comment docs.

[kzc]

The node12/macos race condition test failure in this PR "correctly rewrites change event during build delay" happens roughly 20% of the time on github CI. Although not a proper fix, it might be helped by the timeout increase at the end of the patch #4300 (comment). I don't know why await sander.copydir() is insufficient:

rollup/test/watch/index.js

Lines 309 to 315 in 2a899d5

	it('correctly rewrites change event during build delay', async () => {
	const WATCHED_ID = path.resolve('test/_tmp/input/watched');
	const MAIN_ID = path.resolve('test/_tmp/input/main.js');
	let lastEvent = null;
	await sander.copydir('test/watch/samples/watch-files').to('test/_tmp/input');
	await wait(100);
	watcher = rollup.watch({

The implementation of sander.copydir is quite interesting, and counter-intuitive for me at least. I can only speculate that some of its file write actions are deferred and not accurately awaited.

[dnalborczyk]

I don't know why await sander.copydir() is insufficient:
...
The implementation of sander.copydir is quite interesting, and counter-intuitive for me at least. I can only speculate that some of its file write actions are deferred and not accurately awaited.

might be time to replace sander with fs-extra. sander is unmaintained, last commit was 5+ years ago, 50k downloads/week. fs-extra is actively maintained, with 42 million downloads/week.

I'll have a look into a PR.

[kzc]

There are two predominant flaky tests on macos.

This particular watch test test/cli/samples/watch/watch-config-early-update fails on macos (usually macos/node12, but occasionally on macos/node16) around 20% of the time:

  1 failing

  1) rollup
       cli
         watch
           watch-config-early-update: immediately reloads the config file if a change happens while it is parsed:

      AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ 'output1.js: const foo = 42;\n\nexport { foo };\n'
- 'output1.js: undefined'
               ^
      + expected - actual

      -output1.js: const foo = 42;
      -
      -export { foo };
      +output1.js: undefined

This is the other macos failure that occurs less than 10% of the time on macos/node12, which oddly is reported as 2 test failures by mocha when running test/watch/index.js "correctly rewrites change event during build delay":

  2 failing
npm ERR! code ELIFECYCLE

  1) rollup
npm ERR! errno 2
       rollup.watch
npm ERR! rollup@2.62.0 test:only: `mocha test/test.js`
         correctly rewrites change event during build delay:
npm ERR! Exit status 2
     Uncaught AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
npm ERR! 

npm ERR! Failed at the rollup@2.62.0 test:only script.
'update' !== null
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

      at Object.watchChange (test/watch/index.js:335:14)
      at PluginDriver.runHookSync (src/utils/PluginDriver.ts:357:30)
      at PluginDriver.hookSeqSync (src/utils/PluginDriver.ts:276:9)
      at WatchEmitter.handleChange (src/Graph.ts:82:23)
      at Timeout._onTimeout (src/watch/watch.ts:92:18)
      at listOnTimeout (internal/timers.js:554:17)
      at processTimers (internal/timers.js:497:7)

  2) rollup
       rollup.watch
         correctly rewrites change event during build delay:
     Error: done() called multiple times in test <rollup rollup.watch correctly rewrites change event during build delay> of file /Users/runner/work/rollup/rollup/test/test.js; in addition, done() received error: AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ 'update'
- 'delete'
      at processTicksAndRejections (internal/process/task_queues.js:97:5)

My initial suspicion that it was due to await sander.copydir doesn't appear to be the case: #4319 (comment)

[kzc]

For the second watch failure above, this hack appears to yield positive results on an overloaded macos box:

--- a/test/watch/index.js
+++ b/test/watch/index.js
@@ -309,29 +309,29 @@ describe('rollup.watch', () => {
        it('correctly rewrites change event during build delay', async () => {
                const WATCHED_ID = path.resolve('test/_tmp/input/watched');
                const MAIN_ID = path.resolve('test/_tmp/input/main.js');
                let lastEvent = null;
                await sander.copydir('test/watch/samples/watch-files').to('test/_tmp/input');
                await wait(100);
                watcher = rollup.watch({
                        input: 'test/_tmp/input/main.js',
                        output: {
                                file: 'test/_tmp/output/bundle.js',
                                format: 'cjs',
                                exports: 'auto'
                        },
                        watch: {
-                               buildDelay: 300,
+                               buildDelay: 600,
                                chokidar: {
                                        atomic: false
                                }
                        },
                        plugins: {
                                buildStart() {
                                        this.addWatchFile(WATCHED_ID);
                                },
                                watchChange(id, { event }) {
                                        if (id === WATCHED_ID) {
                                                assert.strictEqual(lastEvent, null);
                                                lastEvent = event;
                                        }
                                }

[lukastaegert]

I added the increased timeout. This is not fixed yet, but I will move it to master as it definitely appears to be an improvement.