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

Fix #411: Tests if event is thrown when calling rename #478

Merged
merged 3 commits into from Oct 15, 2018

Conversation

Projects
None yet
3 participants
@AHKol
Contributor

AHKol commented Sep 24, 2018

Could not make the change event to throw rename during a rename call.

Test will check if change event is thrown when a file is renamed.

@0xazure

This test looks great!

fs.watch() is an interesting one, just because the underlying implementation is pretty platform-specific.

Double-check the output of the automated tests, it's reported the tests succeeded but if you take a look at the output it reports the missing semi-colon I highlighted in my inline comment.

fs.writeFile('/myfile', 'data', function(error) {
if(error) throw error;
})

This comment has been minimized.

@0xazure

0xazure Sep 24, 2018

Contributor

nit: Missing semi-colon.

Running npm test will catch a lot of these issues for you before you submit your PR.

@0xazure

This comment has been minimized.

Contributor

0xazure commented Oct 1, 2018

@AHKol I'm having another look at this and I think I initially misunderstood the filer implementation. Did you see @humphd's comment about the lack of rename events in filer? On NodeJS, rename events are emitted when a file is renamed, so this test should actually be failing.

});
var watcher = fs.watch('/myfile', function(event, filename) {
expect(event).to.equal('change');

This comment has been minimized.

@0xazure

0xazure Oct 1, 2018

Contributor

This should be a rename event, which filer doesn't currently support.

This comment has been minimized.

@humphd

humphd Oct 9, 2018

Contributor

Agree with @0xazure, let's at least document this oddity via a comment above this line.

});
var watcher = fs.watch('/myfile', function(event, filename) {
expect(event).to.equal('change');

This comment has been minimized.

@humphd

humphd Oct 9, 2018

Contributor

Agree with @0xazure, let's at least document this oddity via a comment above this line.

@AHKol

This comment has been minimized.

Contributor

AHKol commented Oct 15, 2018

@humphd Added the requested comment.

@humphd

humphd approved these changes Oct 15, 2018

@humphd humphd merged commit 8aa8dda into filerjs:master Oct 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment