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

Fixed watcher stop CI test on Windows. #2612

Merged

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Feb 1, 2019

↪️ Pull Request

watcher.stop() test failed on Windows 10. Details on #2609.

🤔 What is changed?

In the previous test, it compared the time to check if stop() is called successfully.

After reading child process document and some nodejs code, it is a better solution to check killed rather than using time differences.

Because this.child.killed is set to true when kill or TerminateProcess finished their job correctly.

💻 Examples

N/A

🚨 Test instructions

When added a pull request, it failed.

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@sainthkh
Copy link
Contributor Author

sainthkh commented Feb 1, 2019

Copy link
Member

@DeMoorJasper DeMoorJasper left a comment

Choose a reason for hiding this comment

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

Great research and fix :)

Kinda hoped the entire CI would be a lot more stable now but unfortunately there is still some Red in the CI :(

@DeMoorJasper
Copy link
Member

@devongovett could this be merged?

@devongovett devongovett merged commit 32791e3 into parcel-bundler:master Feb 2, 2019
DeMoorJasper pushed a commit that referenced this pull request Feb 3, 2019
#2605)

* Ignore Kotlin tests and show messages when Java is not installed or configured. (#2603)

* Skip symlink tests and show warning message when tests are run without admin privilege. (#2602)

* Moved parcel-bundler/test/utils.js and integration-tests/test/utils.js into test-utils/src/utils.js. (#2604)

* Fixed eslint errors.

* Use the test to assert this.child.killed rather than checking time difference. (#2609) (#2612)
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