watch: fix watch path with equals by MoLow · Pull Request #47369 · nodejs/node · GitHub

prior to this fix node --watch-path path.js worked but node --watch-path=path.js did not

Is there was no general test for —watch-path both the tests for checking for non-existing files, if there isn't i think we could add a test for —watch-path watching over a directory and executing a file like it is for the —watch tests

Is there was no general test for —watch-path both the tests for checking for non-existing files, if there isn't i think we could add a test for —watch-path watching over a directory and executing a file like it is for the —watch tests

There is such a test. see test/sequential/test-watch-mode.mjs

Is there was no general test for —watch-path both the tests for checking for non-existing files, if there isn't i think we could add a test for —watch-path watching over a directory and executing a file like it is for the —watch tests

There is such a test. see test/sequential/test-watch-mode.mjs

could you point out which ones 😅😅 on test/sequential/test-watch-mode.mjs was able to find two tests:

https://github.com/nodejs/node/blob/5c048889f7258a09d5f41860f34ae2ccd3604070/test/sequential/test-watch-mode.mjs#L158

https://github.com/nodejs/node/blob/5c048889f7258a09d5f41860f34ae2ccd3604070/test/sequential/test-watch-mode.mjs#L140

both of which mention watching non-existing case

Also, I tried running to see if the console log not appearing issue is fixed for example I have a file test.mjs and test_dir/test.js, test.mjs has a statement console.log('hello world'); and running out/Release/node --watch-path=./test_dir test.mjs gave the following output, hello world not printed

I was basically talking of a test something like this

  it('should watch changes to a file with watch-path', {
    skip: !supportsRecursive,
  }, async () => {
    const file = createTmpFile();
    const watchedFile = fixtures.path('watch-mode/subdir/file.js');
    const { stderr, stdout } = await spawnWithRestarts({
      file,
      watchedFile,
      args: ['--watch-path', fixtures.path('./watch-mode/subdir'), file],
    });
    assert.strictEqual(stderr, '');
    assertRestartedCorrectly({
      stdout,
      messages: { inner: 'running', completed: `Completed running ${inspect(file)}`, restarted: `Restarting ${inspect(file)}` },
    });
  });

ok was able make the test case pass with the following changes:

in lib/internal/main/watch_mode.js passing the 'inherit' option when using watch mode

--- a/lib/internal/main/watch_mode.js
+++ b/lib/internal/main/watch_mode.js
@@ -53,7 +53,7 @@ let exited;
 
 function start() {
   exited = false;
-  const stdio = kShouldFilterModules ? ['inherit', 'inherit', 'inherit', 'ipc'] : undefined;
+  const stdio = kShouldFilterModules ? ['inherit', 'inherit', 'inherit', 'ipc'] : 'inherit';
   child = spawn(process.execPath, args, { stdio, env: { ...process.env, WATCH_REPORT_DEPENDENCIES: '1' } });
   watcher.watchChildProcessModules(child);
   child.once('exit', (code) => {

and updated the tests as follows:

--- a/test/sequential/test-watch-mode.mjs
+++ b/test/sequential/test-watch-mode.mjs
@@ -148,7 +165,8 @@ describe('watch mode', { concurrency: false, timeout: 60_000 }, () => {
       args: ['--watch-path', fixtures.path('./watch-mode/subdir/'), file],
     });
 
-    assert.strictEqual(stderr, '');
+    assert.match(stderr, /Error: Cannot find module/);
+    assert(stderr.match(/Error: Cannot find module/g).length >= 2);
     assertRestartedCorrectly({
       stdout,
       messages: { completed: `Failed running ${inspect(file)}`, restarted: `Restarting ${inspect(file)}` },
@@ -166,7 +184,8 @@ describe('watch mode', { concurrency: false, timeout: 60_000 }, () => {
       args: [`--watch-path=${fixtures.path('./watch-mode/subdir/')}`, file],
     });
 
-    assert.strictEqual(stderr, '');
+    assert.match(stderr, /Error: Cannot find module/);
+    assert(stderr.match(/Error: Cannot find module/g).length >= 2);
     assertRestartedCorrectly({
       stdout,
       messages: { completed: `Failed running ${inspect(file)}`, restarted: `Restarting ${inspect(file)}` },

does this look ok? if yes could commit it to this branch @MoLow

thanks @debadree25!
feel free to commit to this branch if you find anything else

just added the test for watch-path in the general case @MoLow

CI: https://ci.nodejs.org/job/node-test-pull-request/50823/

FWIW it seems odd to implement this in JS, separate from the usual C++ argument parsing logic. I had similar concerns in the original PR.

Is there a similar example in present cpp where multiple options are parsed like this?

CI: https://ci.nodejs.org/job/node-test-pull-request/50827/

CI: https://ci.nodejs.org/job/node-test-pull-request/50832/

Landed in 25858e3

PR-URL: #47369 Fixes: #47296 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: nodejs#47369 Fixes: nodejs#47296 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

MoLow requested review from benjamingr and debadree25 April 2, 2023 06:57

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 2, 2023

MoLow added the watch-mode Issues and PRs related to watch mode label Apr 2, 2023

aduh95 force-pushed the main branch from d8e3fca to 85705a4 Compare April 2, 2023 07:26

MoLow force-pushed the fix-watch-path-equals branch from 910d539 to 2d84c7b Compare April 2, 2023 07:34

debadree25 reviewed Apr 2, 2023

View reviewed changes

Comment thread test/sequential/test-watch-mode.mjs Outdated

debadree25 reviewed Apr 2, 2023

View reviewed changes

MoLow force-pushed the fix-watch-path-equals branch from 2d84c7b to 5c04888 Compare April 2, 2023 11:13

watch: fix watch path with equals

1c4d92c

MoLow force-pushed the fix-watch-path-equals branch from 5c04888 to 1c4d92c Compare April 2, 2023 16:51

fixup! add a test for --watch-path

cec5b36

debadree25 approved these changes Apr 2, 2023

View reviewed changes

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 2, 2023

tniessen reviewed Apr 2, 2023

View reviewed changes

github-actions Bot mentioned this pull request Apr 3, 2023

CI Reliability 2023-04-03 nodejs/reliability#538

Open

28 tasks

github-actions Bot mentioned this pull request Apr 4, 2023

CI Reliability 2023-04-04 nodejs/reliability#539

Open

29 tasks

benjamingr approved these changes Apr 5, 2023

View reviewed changes

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 5, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 5, 2023

nodejs-github-bot merged commit 25858e3 into nodejs:main Apr 5, 2023

RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023

watch: fix watch path with equals

e0ec04b

PR-URL: #47369 Fixes: #47296 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

RafaelGSS mentioned this pull request Apr 6, 2023

v19.9.0 proposal #47441

Merged

RafaelGSS pushed a commit that referenced this pull request Apr 6, 2023

watch: fix watch path with equals

21e1e1b

PR-URL: #47369 Fixes: #47296 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

MoLow deleted the fix-watch-path-equals branch April 6, 2023 18:15

RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023

watch: fix watch path with equals

633209c

PR-URL: #47369 Fixes: #47296 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

RafaelGSS pushed a commit that referenced this pull request Apr 8, 2023

watch: fix watch path with equals

75669e9

PR-URL: #47369 Fixes: #47296 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

RafaelGSS pushed a commit that referenced this pull request Apr 13, 2023

watch: fix watch path with equals

fb047d6

PR-URL: #47369 Fixes: #47296 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

danielleadams pushed a commit that referenced this pull request Jul 6, 2023

watch: fix watch path with equals

6ca388d

PR-URL: #47369 Fixes: #47296 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

MoLow added a commit to MoLow/node that referenced this pull request Jul 6, 2023

watch: fix watch path with equals

d3e0229

PR-URL: nodejs#47369 Fixes: nodejs#47296 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

watch: fix watch path with equals by MoLow · Pull Request #47369 · nodejs/node · GitHub

Uh oh!

Uh oh!

watch: fix watch path with equals - #47369

watch: fix watch path with equals#47369nodejs-github-bot merged 2 commits intonodejs:mainnodejs/node:mainfrom MoLow:fix-watch-path-equalsMoLow/node:fix-watch-path-equalsCopy head branch name to clipboard

Conversation

MoLow commented Apr 2, 2023

Uh oh!

Uh oh!

debadree25 left a comment

Choose a reason for hiding this comment

Uh oh!

MoLow commented Apr 2, 2023

Uh oh!

debadree25 commented Apr 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

debadree25 commented Apr 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

debadree25 commented Apr 2, 2023

Uh oh!

debadree25 commented Apr 2, 2023

Uh oh!

MoLow commented Apr 2, 2023

Uh oh!

debadree25 commented Apr 2, 2023

Uh oh!

nodejs-github-bot commented Apr 2, 2023

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

debadree25 commented Apr 2, 2023

Uh oh!

nodejs-github-bot commented Apr 2, 2023

Uh oh!

nodejs-github-bot commented Apr 2, 2023

Uh oh!

nodejs-github-bot commented Apr 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

watch: fix watch path with equals#47369
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
MoLow:fix-watch-path-equals

debadree25 commented Apr 2, 2023 •
edited
Loading

debadree25 commented Apr 2, 2023 •
edited
Loading