test: add block-scoping to test-fs-watch-encoding

[LakshmiSwethaG]

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[sam-github]

Hi Lakshmi, thanks for the contribution, but I'm not in favour. Rockets (() => {} ) are useful for inline functions, and while it is possible to create a rocket, then assign it to a const to emulate the function ... syntax, its less clear when used like this, and even textually longer. I don't think this is a class of change we want to make in node. There are 790+ instances of this in test/parallel alone, much less the rest of node, but I'm not aware of any cases of "const rockets" at the file scope.

There are lots of things that can be improved about our test suites, maybe we can find some other kinds of refactorings you would be interested in working on?

[Trott]

@LakshmiSwethaG Welcome and thanks for the pull request! I agree with @sam-github that this is not a desirable change. However, instead, in the same file, perhaps you can refactor it to block-scope things so that we don't have watcher1, watcher2, and watcher3?

For example, instead of this:

const watcher1 = fs.watch(
  tmpdir.path,
  { encoding: 'hex' },
  (event, filename) => {
    if (['e696b0e5bbbae69687e5a4b9e4bbb62e747874', null].includes(filename))
      done(watcher1);
  }
);
registerWatcher(watcher1);

...it could be block-scoped like this:

{
  const watcher = fs.watch(
    tmpdir.path,
    { encoding: 'hex' },
    (event, filename) => {
      if (['e696b0e5bbbae69687e5a4b9e4bbb62e747874', null].includes(filename))
        done(watcher);
    }
  );
  registerWatcher(watcher);
}

...and similarly for watcher2 and watcher3.

Then, a comment can be added to each of the three block-scopes explaining exactly what it is testing. One short sentence or sentence fragment in each case should be enough. So the block above, might have this comment added to it:

// Test that using the `encoding` option has the expected result.

[LakshmiSwethaG]

@sam-github and @Trott Thanks for your suggestions. I have updated the commit with @Trott suggestions. Please review and suggest.
Sorry for the delay in response I didn't have access to any system.

PR is completely different from initial PR. Needs re-review. Dismissing all existing reviews.

[Trott]

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2406/

[Trott]

Full CI: https://ci.nodejs.org/job/node-test-pull-request/20368/ ✅

[Trott]

Should probably wait 48 hours from #25532 (comment) before landing. Otherwise, this seems good to land (unless someone notices something and comments between now and then).

[addaleax]

Landed in 0f0e7f5 🎉