GeezerVison

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.68%. Comparing base (40c625b) to head (1dcd042).
⚠️ Report is 1266 commits behind head on main.

@@            Coverage Diff             @@
##             main   #60553      +/-   ##
==========================================
- Coverage   89.68%   89.68%   -0.01%     
==========================================
  Files         676      676              
  Lines      206576   206589      +13     
  Branches    39556    39564       +8     
==========================================
+ Hits       185266   185273       +7     
  Misses      13445    13445              
- Partials     7865     7871       +6     

cc @jasnell @Qard @santigimeno @anonrig as reviewers of #49884

/cc @nodejs/fs

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

Lint CI didn't fail at the time of filing this PR but fails now, apparently after #60845 landed before this PR and changed lint config

If someone understands what's up with lint and could fix that (or file an alternative PR) it'd be great, otherwise I'll get back to this at some point later

That looks like a false positive of the new rule? Asserts are immediately called, they are not passed to a callback.

cc @aduh95 perhaps

It sounds like the linter was previously not catching something it should have (and is now doing).

I think it's the checkCalls wrapper creating to necessity for mustCall / mustSucceed.

Why should it and what does that do?

something it should have

Why should it be?

It's an assertion helper which is immediately used in linear code, not a callback.
There is no use in wrapping it, it doesn't add any reasonable guards on top of an unused function lint check.

Doing that just to silence lint seems wrong

To somewhat articulate: it adds as much value as wrapping every line in mustCall(() => { ...line })(); in linear code.

And if it should be wrapped, then with what? Should it also specify how many times that is expected to be used?

I think it's the checkCalls wrapper creating to necessity for mustCall / mustSucceed.

Or I don't understand. Is mustCall supposed to be used inside the wrapper? How?

No way I can think of makes sense, if that's not a false positive, I'm not sure what's that supposed to do at all.
This also seems undocumented

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

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

You need to either drop the checkCalls and replace it with a loop, or wrap it in common.mustCallAtLeast

@gurgunday what's the point of you restarting the CI here? There are failures to address, please don't resume jobs if it has failures related to the PR.

@aduh95 is there a way to reconfigure the lint rule so it won't require useless common.mustCallAtLeast wraps on non-callbacks?

@aduh95 my bad I thought it was addressed

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

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

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

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

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

This pull request has been automatically closed after 30 days of inactivity following its stale status (no activity for a total of 120 days).
If this is still relevant, feel free to reopen it or leave a comment with additional details so we can continue the discussion.