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

doc, test: update --watch for linux recursive watch #55256

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented Oct 3, 2024

Follow-up #45098

This PR removes the unused error code ERR_FEATURE_UNAVAILABLE_ON_PLATFORM, and stops skipping some of the watch mode tests on Linux machines.

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. labels Oct 3, 2024
@richardlau
Copy link
Member

I'm a bit surprised ERR_FEATURE_UNAVAILABLE_ON_PLATFORM is now unused -- I was under the impression that watch mode still doesn't work on e.g. IBM i (most watch tests skip) and at one point that error was returned there.

cc @nodejs/platform-ibmi

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Oct 3, 2024

If you search for the error, it's only mentioned in the docs and defined in errors.js, it is never thrown.

@richardlau
Copy link
Member

If you search for the error, it's only mentioned in the docs and defined in errors.js, it is never thrown.

I can see that, but I'm questioning whether the code was changed to not throw the error anymore on all platforms despite watch not working on IBM i -- AFAIK it's a system limitation there.

@RedYetiDev RedYetiDev added the watch-mode Issues and PRs related to watch mode label Oct 3, 2024
Copy link

codecov bot commented Oct 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.41%. Comparing base (1d5ed72) to head (a0719b8).
Report is 237 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55256      +/-   ##
==========================================
- Coverage   88.41%   88.41%   -0.01%     
==========================================
  Files         652      652              
  Lines      186572   186585      +13     
  Branches    36045    36061      +16     
==========================================
+ Hits       164957   164967      +10     
+ Misses      14899    14887      -12     
- Partials     6716     6731      +15     
Files with missing lines Coverage Δ
lib/internal/errors.js 96.98% <ø> (-0.01%) ⬇️

... and 49 files with indirect coverage changes

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Oct 4, 2024

FWIW the tests are skipped on IBMi

@abmusse
Copy link
Contributor

abmusse commented Oct 7, 2024

@richardlau

Looks like calling fs.watch on IBM i returns ENOSYS.

const fs = require('fs');

fs.watch('dummy', (eventType, filename) => {
  console.log(`event type is: ${eventType}`);
  if (filename) {
    console.log(`filename provided: ${filename}`);
  } else {
    console.log('filename not provided');
  }
});
Error: ENOSYS: function not implemented, watch 'dummy'

I'm not sure if ever returned ERR_FEATURE_UNAVAILABLE_ON_PLATFORM in the past but currently ENOSYS is being returned.

@richardlau
Copy link
Member

Maybe I misremembered then. 🤷

@RedYetiDev RedYetiDev added linux Issues and PRs related to the Linux platform. doc Issues and PRs related to the documentations. labels Oct 14, 2024
@RedYetiDev RedYetiDev added review wanted PRs that need reviews. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. labels Nov 2, 2024
@RedYetiDev
Copy link
Member Author

RedYetiDev commented Nov 3, 2024

I've added https://github.com/nodejs/node/labels/dont-land-on-v18.x as #45098 didn't land on that release line. If you could review it would be appreciated :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. errors Issues and PRs related to JavaScript errors originated in Node.js core. linux Issues and PRs related to the Linux platform. needs-ci PRs that need a full CI run. review wanted PRs that need reviews. watch-mode Issues and PRs related to watch mode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants