Stabilize watch tests by lukastaegert · Pull Request #4318 · rollup/rollup

lukastaegert · 2021-12-25T05:29:30Z

This PR contains:

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 · 2021-12-25T05:31:13Z

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 · 2021-12-25T05:38:48Z

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 · 2021-12-25T16:22:07Z

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 · 2021-12-25T20:22:23Z

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 · 2021-12-26T18:43:10Z

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 · 2021-12-26T23:27:08Z

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 · 2021-12-30T06:43:23Z

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.

lukastaegert mentioned this pull request Dec 25, 2021

chore: bump deps, fix expected test result for core-js #4300

Merged

9 tasks

lukastaegert force-pushed the stabilize-watch-tests branch 3 times, most recently from ef1808d to cf3de24 Compare December 25, 2021 05:36

Use atomic file writes

1a84b57

lukastaegert force-pushed the stabilize-watch-tests branch from cf3de24 to 1a84b57 Compare December 25, 2021 05:45

Use atomic file writes for regular watch tests

22bdda0

kzc mentioned this pull request Dec 26, 2021

refactor: use fs, fs-extra, remove sander #4319

Merged

9 tasks

kzc mentioned this pull request Dec 27, 2021

Detect unfulfilled async hook actions and report error on exit #4320

Merged

9 tasks

Increase timeout

9c008de

lukastaegert marked this pull request as ready for review December 30, 2021 06:42

lukastaegert merged commit 7f55571 into master Dec 30, 2021

lukastaegert deleted the stabilize-watch-tests branch December 30, 2021 06:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Stabilize watch tests#4318

Stabilize watch tests#4318
lukastaegert merged 3 commits intomasterfrom
stabilize-watch-tests

lukastaegert commented Dec 25, 2021

Uh oh!

github-actions bot commented Dec 25, 2021 •

edited

Loading

Uh oh!

codecov bot commented Dec 25, 2021 •

edited

Loading

Uh oh!

kzc commented Dec 25, 2021 •

edited

Loading

Uh oh!

dnalborczyk commented Dec 25, 2021

Uh oh!

kzc commented Dec 26, 2021

Uh oh!

kzc commented Dec 26, 2021

Uh oh!

lukastaegert commented Dec 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

lukastaegert commented Dec 25, 2021

Description

Uh oh!

github-actions bot commented Dec 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thank you for your contribution! ❤️

Uh oh!

codecov bot commented Dec 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kzc commented Dec 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnalborczyk commented Dec 25, 2021

Uh oh!

kzc commented Dec 26, 2021

Uh oh!

kzc commented Dec 26, 2021

Uh oh!

lukastaegert commented Dec 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Dec 25, 2021 •

edited

Loading

codecov bot commented Dec 25, 2021 •

edited

Loading

kzc commented Dec 25, 2021 •

edited

Loading