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 tests to narrowly demonstrate defect mentioned in PR #682 #737

Merged
merged 6 commits into from
Jun 19, 2018

Conversation

srguiwiz
Copy link
Contributor

This isn't a fix yet, but more narrowly demonstrates scope of defect mentioned in PR #682.

The referenced broadly failing test could create an impression this software library had gotten worse. In fact, it hadn't gotten worse. These tests now spell out more narrowly what still works as always and continues to work, versus what is broken and apparently had been broken for some time.

@coveralls
Copy link

coveralls commented Jun 18, 2018

Coverage Status

Coverage decreased (-0.1%) to 96.208% when pulling 5966686 on srguiwiz:fix-issue-682 into 9ac1ffd on paulmillr:master.

@es128
Copy link
Collaborator

es128 commented Jun 18, 2018

This looks excellent, thank you. However, I'm not so sure about that last commit. We want ignoreInitial for these tests to ensure that events coming before the ready event do not interfere with the spies

Also, put back previously removed options.ignoreInitial in tests.
Copy link
Collaborator

@es128 es128 left a comment

Choose a reason for hiding this comment

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

Just trying to make the test descriptions a bit clearer, otherwise looks good.

test.js Outdated
@@ -636,12 +636,97 @@ function runTests(baseopts) {
});
});
});
it('should detect unlink unfazed by in other directory watching a second file that does not exist', function(done) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

little phrasing tweaks

should detect unlink while watching a non-existent second file in another directory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

implemented all phrase changes as requested, thank you

test.js Outdated
}));
});
});
it('should detect unlink and re-add unfazed by watching a second file', function(done) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should detect unlink and re-add while watching a second file

test.js Outdated
}));
});
});
it('should detect unlink and re-add unfazed by in other directory watching a second file that does not exist', function(done) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should detect unlink and re-add while watching a non-existent second file in another directory

test.js Outdated
}));
});
});
it('should detect unlink and re-add unfazed by in same directory watching a second file that does not exist', function(done) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should detect unlink and re-add while watching a non-existent second file in the same directory

In this case a watched file is being unlinked and re-added, while
another watched file is not unlinked, and one watched file does not exist.
Copy link
Contributor Author

@srguiwiz srguiwiz left a comment

Choose a reason for hiding this comment

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

made changes as requested, thank you too

@srguiwiz
Copy link
Contributor Author

The very last test added originated from a thought experiment. Admittedly, it didn't prove a suspicion. Nevertheless, a correct implementation should pass that test too. A good fix should pass this test without a problem, a bad fix would have to fear it.

This one in 2.0.4 fails non-polling yet passes polling.
@srguiwiz
Copy link
Contributor Author

A main objective here was to make clear there was no regression. Seeing 2.0.4 released, that goal appears achieved.

There is "only" a newly discovered defect.

The additional test cases here I have added for sanity, as guidance for implementing a fix.

I have tried implementing a fix too, as a public service, but haven't figured one.

Watching individual files is not a current use case for our work, so this is not a priority for me, and I will not proceed trying to implement a fix, not at this time.

@es128 es128 merged commit b1f8bcb into paulmillr:master Jun 19, 2018
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.

None yet

3 participants