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

fix: watch does not work for glob pattern '<dir>/**' if '<dir>' does not exist on fs #746

Closed
wants to merge 1 commit into from

Conversation

eGavr
Copy link

@eGavr eGavr commented Jul 14, 2018

Resolves #745

@coveralls
Copy link

coveralls commented Jul 14, 2018

Coverage Status

Coverage increased (+0.05%) to 96.255% when pulling af71c17 on eGavr:issue/745 into 8cd5f7a on paulmillr:master.

@srguiwiz
Copy link
Contributor

srguiwiz commented Jul 15, 2018

@eGavr , thank you for taking initiative and fixing.

Testing is necessary. Testing this project for now is a little bit odd, because the test suite has cases in it that don't pass, for yet to be fixed defects.

In order to ensure not to break existing functionality with a next release, please:

  • Run test suite with your fix not in (previous commit), note what fails.
  • Run test suite with your fix. Compare what fails then.

That protects quality and sanity.

To help prevent future regression, you could also add a test case in test.js that fails before your fix and then passes with your fix.

@eGavr eGavr force-pushed the issue/745 branch 2 times, most recently from c1ec6c1 to ddf8741 Compare July 16, 2018 09:39
@eGavr
Copy link
Author

eGavr commented Jul 16, 2018

@srguiwiz

I've created a testing pull request #749 where I've just changed README and you can see in it that there are 7 broken tests.

In my PR there were 7 broken tests too.
Then I've wrote a test on my changed, push it and I see that there are 6 broken tests for now :)

I think that some tests are not stable and fail randomly.

Can you review my changes, please? It seems that they do not break the current functionality and fix the problem which I've described in the issue.

@eGavr
Copy link
Author

eGavr commented Jul 16, 2018

For now there are 9 broken tests :(

The previous commit which is different from the latest one only in name of the test which I've written has 7 broken tests.

@eGavr
Copy link
Author

eGavr commented Jul 17, 2018

ping :)

@paulmillr paulmillr closed this Mar 22, 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.

None yet

4 participants