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

Issue 627 #746

Merged
merged 8 commits into from
Mar 12, 2019
Merged

Issue 627 #746

merged 8 commits into from
Mar 12, 2019

Conversation

CDNRae
Copy link
Contributor

@CDNRae CDNRae commented Mar 6, 2019

Fixes #627 by adding 3 tests to fs.write():

  • Test that tries to use a non-existent fd
  • Test that tries to open a file without the O_WRITE flag and then call fs.write
  • Test that tries to write a buffer whose length is too small (buffer.length - offset < length)

@codecov-io
Copy link

codecov-io commented Mar 6, 2019

Codecov Report

Merging #746 into master will increase coverage by 0.17%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #746      +/-   ##
==========================================
+ Coverage   86.71%   86.88%   +0.17%     
==========================================
  Files          16       16              
  Lines        1739     1739              
==========================================
+ Hits         1508     1511       +3     
+ Misses        231      228       -3
Impacted Files Coverage Δ
src/filesystem/implementation.js 84.13% <0%> (+0.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94d5ff8...01f3f1c. Read the comment docs.

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great to get extra test coverage. I've included a few requests to make the checks more specific, and to clarify some things. Should be good to go after these fixes.

Thanks for working on these tests.

tests/spec/fs.write.spec.js Outdated Show resolved Hide resolved
tests/spec/fs.write.spec.js Outdated Show resolved Hide resolved
tests/spec/fs.write.spec.js Outdated Show resolved Hide resolved
tests/spec/fs.write.spec.js Show resolved Hide resolved
… is too long, check the type of error that a test returns
@CDNRae
Copy link
Contributor Author

CDNRae commented Mar 11, 2019

Made the changes -- hope these are good to go! Thanks for reviewing my work!

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is looking good, thanks for making these fixes. I have one more I'd ask that you do, then I think it's ready.

const fs = util.fs();
const buffer = Buffer.from([1, 2, 3, 4, 5, 6, 7, 8]);

fs.write('/myfile', buffer, 0, buffer.length, 0, function(error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fs.write takes an fd, so passing it a filepath as a string is really testing what we do when we don't pass an fd. If I do this in node, it throws. Probably returning EBADF is OK here, since it literally is a bad file descriptor.

That said, let's adjust the message on line 69 to be more clear: it('should fail if given a file path vs. an fd'

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks for working through these reviews.

@humphd humphd merged commit 4aae538 into filerjs:master Mar 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants