Node: fs/promises test coverage is poor.

[ChALkeR]

Experimental fs/promises API is present in the current master without proper test coverage.

According to the coverage report, the following functions are not tested at all:

• fd.chown,
• fd.datasync,
• fd.sync,
• fd.truncate,
• fd.utimes,
• truncate,
• readlink,
• symlink,
• lstat,
• lchmod,
• lchown,
• fchown,
• chown,
• realpath.

19811 adds a few tests, but that's still not near being complete even after that lands.

Note that tests for lchown should probably be landed _after_ #20407 as that method is currently broken (until #20407 lands).

/cc @mscdex @davisjam @nodejs/testing

[ChALkeR]

@benjamingr is this suitable for good first issue?

[TimothyGu]

Ideally we should have a shared test suite between the callback-based functions and the promise-based ones. fs/promises would never be truly a first-class citizen if its tests are not as comprehensive as the callback-based functions.

[benjamingr]

@ChALkeR I think so - thanks :) I'm doing an onboarding event on May 15th and I could make writing tests for fs promises functions into a good "first Node.js PR" exercise.

[davisjam]

shared test suite between the callback-based functions and the promise-based ones

I agree this seems like the right direction. Not re-implementing tests, but figuring out how to get callbacks and promises to share tests.

[jasnell]

It's been an outstanding to-do of mine that's not on my schedule to get back to until later in May. If others can jump in and help fill in gaps I'd certainly appreciate it.

[Trott]

Ideally we should have a shared test suite between the callback-based functions and the promise-based ones. fs/promises would never be truly a first-class citizen if its tests are not as comprehensive as the callback-based functions.

I agree this seems like the right direction. Not re-implementing tests, but figuring out how to get callbacks and promises to share tests.

It's been an outstanding to-do of mine that's not on my schedule to get back to until later in May. If others can jump in and help fill in gaps I'd certainly appreciate it.

@TimothyGu @davisjam @jasnell @ChALkeR Proof-of-concept to kick around: https://github.com/nodejs/node/pull/20439

[ChALkeR]

Was: https://coverage.nodejs.org/coverage-b55a11d1b17b3e4b/root/fs/promises.js.html
Now: https://coverage.nodejs.org/coverage-4b00c4fafaa2ae8c/root/internal/fs/promises.js.html

It became much better, but not complete yet — there are still methods without any tests.

[benjamingr]

Going to have new contributors work on this today @ChALkeR :)

[benjamingr]

Going to ask them to PR into @Trott 's branch since it looks like a nicer approach

[Trott]

Going to ask them to PR into @Trott 's branch since it looks like a nicer approach

There seems to be some question as to whether it is the right approach and I've started picking things off independently more like in #20739 and #20667.

[benjamingr]

Yeah, I've told people to do that instead - was slightly confusing but people were able to contribute to other things in the meantime so no harm done :)

[ChALkeR]

Of the mentioned above modules, only chown/fchown remains uncovered in the latest report and #20574 should fix that.

Closing this, no reason for a meta issue anymore.

[Trott]

Closing this, no reason for a meta issue anymore.

Might want to consider opening one for dns.promises though...

[shisama]

Might want to consider opening one for dns.promises though...

Some methods remain uncovered in the latest report.
I'm increasing coverage for dns.promises. #21559 adds tests for onlookup() and onlookupall(). I'm going to add tests for others.