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

add test for fs.open with wx flag for existing file #745

Merged
merged 2 commits into from
Feb 27, 2019
Merged

add test for fs.open with wx flag for existing file #745

merged 2 commits into from
Feb 27, 2019

Conversation

cdrani
Copy link
Contributor

@cdrani cdrani commented Feb 26, 2019

Fixes #623

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.

@cdrani thanks for this! I checked what node does, and I see it returns an EEXIST vs. ENOENT.

I think we're probably doing the wrong there here: https://github.com/filerjs/filer/blob/master/src/filesystem/implementation.js#L623, and should be using new Errors.EEXIST(...).

Do you want to change what we're doing in implementation and update this test?

@cdrani
Copy link
Contributor Author

cdrani commented Feb 26, 2019

Sure, no problem. I assume I should run build-tests after making this change before running test?

@cdrani
Copy link
Contributor Author

cdrani commented Feb 26, 2019

The test is failing, expecting 'EEXIST' to equal 'ENOENT'.

@humphd
Copy link
Contributor

humphd commented Feb 26, 2019

You can just run npm test and it will rebuild automatically. Did you change your test file to expect EEXIST after making that change to the implementation?

Also, can you push your changes up again and I'll review again, and help fix anything.

@cdrani
Copy link
Contributor Author

cdrani commented Feb 27, 2019

Yes, I made all the changes. Did not notice that somehow that fs.open.spec was placed in readonly mode by vim. 😅 . Tests are passing, will push now.

@codecov-io
Copy link

codecov-io commented Feb 27, 2019

Codecov Report

Merging #745 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #745      +/-   ##
==========================================
+ Coverage   86.65%   86.71%   +0.05%     
==========================================
  Files          16       16              
  Lines        1739     1739              
==========================================
+ Hits         1507     1508       +1     
+ Misses        232      231       -1
Impacted Files Coverage Δ
src/filesystem/implementation.js 83.86% <100%> (+0.09%) ⬆️

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 914ba8b...2bf1aa7. Read the comment docs.

@cdrani
Copy link
Contributor Author

cdrani commented Feb 27, 2019

Concerning this

No binary for FirefoxHeadless browser on your platform.
Please, set "FIREFOX_BIN" env variable.

I noticed that I have ChromeHeadless, but not FirefoxHeadless. How are these setup? Why do I have one binary and not the other?

@humphd
Copy link
Contributor

humphd commented Feb 27, 2019

@cdrani do you have Firefox installed on your machine? That should be enough to make this work.

At any rate, this looks great, nice work. I'm going to merge.

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.

Excellent.

@humphd humphd merged commit ee56794 into filerjs:master Feb 27, 2019
@cdrani
Copy link
Contributor Author

cdrani commented Feb 27, 2019

Oh, I use the developer version. That might be the issue. Thanks for the help!

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