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

watch: fix reloading for rebuild/compiled files #45259

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion lib/internal/main/watch_mode.js
Expand Up @@ -93,7 +93,6 @@ function reportGracefulTermination() {
}

async function stop() {
watcher.clearFileFilters();
const clearGraceReport = reportGracefulTermination();
await killAndWait();
clearGraceReport();
Expand Down
38 changes: 37 additions & 1 deletion test/sequential/test-watch-mode.mjs
Expand Up @@ -6,9 +6,10 @@ import path from 'node:path';
import { execPath } from 'node:process';
import { describe, it } from 'node:test';
import { spawn } from 'node:child_process';
import { writeFileSync, readFileSync } from 'node:fs';
import { writeFileSync, rmSync, readFileSync } from 'node:fs';
import { inspect } from 'node:util';
import { once } from 'node:events';
import { setTimeout } from 'node:timers/promises';

if (common.isIBMi)
common.skip('IBMi does not support `fs.watch()`');
Expand Down Expand Up @@ -174,6 +175,41 @@ describe('watch mode', { concurrency: true, timeout: 60_0000 }, () => {
});
});

it('should watch changes to previously loaded dependencies', async () => {
const dependencyContent = 'module.exports = {}';
const dependency = createTmpFile(dependencyContent);
const relativeDependencyPath = `./${path.basename(dependency)}`;
Copy link
Member

Choose a reason for hiding this comment

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

does path.relative(path.basename(dependency)) produces the same result for this line? if yes, can we use it?

const dependant = createTmpFile(`console.log(require('${relativeDependencyPath}'))`);

let stderr = '';
let stdout = '';
const child = spawn(execPath, ['--watch', '--no-warnings', dependant], { encoding: 'utf8' });
child.stdout.on('data', (data) => { stdout += data; });
child.stderr.on('data', (data) => { stderr += data; });
child.on('error', (err) => { throw err; });
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
child.on('error', (err) => { throw err; });
child.on('error', common.mustNotCall());


await once(child.stdout, 'data');
rmSync(dependency, { force: true });

await setTimeout(600); // throttle + 100
Copy link
Member

Choose a reason for hiding this comment

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

Can you use common.platformTimeout(600) for all values for setTimeout?

writeFileSync(dependency, dependencyContent);

await setTimeout(600); // throttle + 100
child.kill();
await once(child, 'exit');

// TODO(ruyadorno): fs.watch is flaky when removing files from a watched
Copy link
Member

Choose a reason for hiding this comment

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

Can you create an issue for this and assign it to me? And can you give platform information for this line, since fs.watch has different implementations on different OS

// folder, in this test we want to assert the expected reload happened
// after a missing file is recreated so we'll skip the assertions in case
// the expected reload+failure never happened.
// This should be an assertion for the failure instead of an if block once
// the source of this flakyness gets resolved.
if (stdout.match(/Failed/g)?.length) {
assert.strictEqual(stdout.split('Failed')[1].match(/Restarting/g)?.length, 1, new Error('should have restarted after fail'));
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add an assert for the length of split('Failed') operation?

assert.ok(stderr.match(/MODULE_NOT_FOUND/g)?.length, new Error('should have failed for not finding the removed file'));
Comment on lines +207 to +209
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (stdout.match(/Failed/g)?.length) {
assert.strictEqual(stdout.split('Failed')[1].match(/Restarting/g)?.length, 1, new Error('should have restarted after fail'));
assert.ok(stderr.match(/MODULE_NOT_FOUND/g)?.length, new Error('should have failed for not finding the removed file'));
if (stdout.includes('Failed')) {
assert.ok(stdout.split('Failed')[1].includes('Restarting'), new Error('should have restarted after fail'));
assert.ok(stderr.includes('MODULE_NOT_FOUND'), new Error('should have failed for not finding the removed file'));

We should use %String.prototype.includes% here instead because we're only checking if those values are included, although for the first assert we can keep it as is if we really need to check that Restarting only appears once.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or assert.match, to get a helpful string comparison between what was expected in case the test fails.

}
});

it('should restart multiple times', async () => {
const file = createTmpFile();
const { stderr, stdout } = await spawnWithRestarts({ file, restarts: 3 });
Expand Down