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

fs/promises test coverage is poor. #20435

Closed
ChALkeR opened this issue Apr 30, 2018 · 14 comments
Closed

fs/promises test coverage is poor. #20435

ChALkeR opened this issue Apr 30, 2018 · 14 comments
Assignees

Comments

@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Apr 30, 2018

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

This comment has been minimized.

Copy link
Member Author

@ChALkeR ChALkeR commented Apr 30, 2018

@benjamingr is this suitable for good first issue?

@TimothyGu

This comment has been minimized.

Copy link
Member

@TimothyGu TimothyGu commented Apr 30, 2018

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.

@ChALkeR ChALkeR mentioned this issue Apr 30, 2018
2 of 4 tasks complete
@benjamingr

This comment has been minimized.

Copy link
Member

@benjamingr benjamingr commented Apr 30, 2018

@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.

@benjamingr benjamingr self-assigned this Apr 30, 2018
@davisjam

This comment has been minimized.

Copy link
Contributor

@davisjam davisjam commented Apr 30, 2018

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

This comment has been minimized.

Copy link
Member

@jasnell jasnell commented Apr 30, 2018

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

This comment has been minimized.

Copy link
Member

@Trott Trott commented Apr 30, 2018

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: #20439

@ChALkeR ChALkeR added the experimental label May 8, 2018
@ChALkeR

This comment has been minimized.

Copy link
Member Author

@ChALkeR ChALkeR commented May 15, 2018

@benjamingr

This comment has been minimized.

Copy link
Member

@benjamingr benjamingr commented May 15, 2018

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

@benjamingr

This comment has been minimized.

Copy link
Member

@benjamingr benjamingr commented May 15, 2018

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

@Trott

This comment has been minimized.

Copy link
Member

@Trott Trott commented May 15, 2018

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

This comment has been minimized.

Copy link
Member

@benjamingr benjamingr commented May 16, 2018

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

This comment has been minimized.

Copy link
Member Author

@ChALkeR ChALkeR commented Jul 10, 2018

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.

@ChALkeR ChALkeR closed this Jul 10, 2018
@Trott

This comment has been minimized.

Copy link
Member

@Trott Trott commented Jul 10, 2018

Closing this, no reason for a meta issue anymore.

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

@shisama

This comment has been minimized.

Copy link

@shisama shisama commented Jul 11, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.