filerjs / filer

Node-like file system for browsers
BSD 2-Clause "Simplified" License
617 stars 154 forks source link

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

Closed cdrani closed 5 years ago

cdrani commented 5 years ago

Fixes #623

cdrani commented 5 years ago

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

cdrani commented 5 years ago

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

humphd commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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%) :arrow_up:

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 commented 5 years ago

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 commented 5 years ago

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

cdrani commented 5 years ago

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