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

Use @atom/temp instead of node-temp #1225

Merged
merged 6 commits into from Mar 22, 2018

Conversation

Projects
None yet
4 participants
@daviwil
Member

daviwil commented Feb 22, 2018

This change switches the use of the node-temp module to our
@atom/temp fork so that we can benefit from a newer rimraf
version that will resolve some recurring test failures on
AppVeyor.

Resolves #1203.

@daviwil daviwil requested review from BinaryMuse and 50Wliu Feb 22, 2018

@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil

daviwil Feb 22, 2018

Member

Anything else in #1203 that I should address? @50Wliu any suggestion on where I should call temp.cleanupSync() so that the graphical runner cleans up correctly?

Member

daviwil commented Feb 22, 2018

Anything else in #1203 that I should address? @50Wliu any suggestion on where I should call temp.cleanupSync() so that the graphical runner cleans up correctly?

@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil

daviwil Feb 23, 2018

Member

Still seeing the same problem. I think there's a conflict between rimraf and pathwatcher, will add more context to the original issue.

Member

daviwil commented Feb 23, 2018

Still seeing the same problem. I think there's a conflict between rimraf and pathwatcher, will add more context to the original issue.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Feb 23, 2018

Member

any suggestion on where I should call temp.cleanupSync() so that the graphical runner cleans up correctly?

No, I don't. Cleanup should be handled automatically by temp.track() but it seems like closing the test window doesn't fire the exit event.

Member

50Wliu commented Feb 23, 2018

any suggestion on where I should call temp.cleanupSync() so that the graphical runner cleans up correctly?

No, I don't. Cleanup should be handled automatically by temp.track() but it seems like closing the test window doesn't fire the exit event.

@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil

daviwil Feb 23, 2018

Member

Currently trying to switch over to Atom's new watchPath API to avoid the issues with pathwatcher locking recursive directory paths, should have an update later today!

Member

daviwil commented Feb 23, 2018

Currently trying to switch over to Atom's new watchPath API to avoid the issues with pathwatcher locking recursive directory paths, should have an update later today!

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Feb 27, 2018

Member

This will also close #796 again because we can upgrade tree-view on atom/atom again.

Member

Ben3eeE commented Feb 27, 2018

This will also close #796 again because we can upgrade tree-view on atom/atom again.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Mar 6, 2018

Member

Interestingly, specs no longer crash on my machine even without these changes. I wonder if there's a change in Atom master not yet on beta that helped with the crashes?

Member

50Wliu commented Mar 6, 2018

Interestingly, specs no longer crash on my machine even without these changes. I wonder if there's a change in Atom master not yet on beta that helped with the crashes?

@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil

daviwil Mar 6, 2018

Member

Hmmm, I wonder if we can run tree-view specs against Atom Dev in CI to verify?

Member

daviwil commented Mar 6, 2018

Hmmm, I wonder if we can run tree-view specs against Atom Dev in CI to verify?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Mar 7, 2018

Member

Hmmm, I wonder if we can run tree-view specs against Atom Dev in CI to verify?

Still crashes :/.

Member

50Wliu commented Mar 7, 2018

Hmmm, I wonder if we can run tree-view specs against Atom Dev in CI to verify?

Still crashes :/.

daviwil and others added some commits Feb 22, 2018

Use @atom/temp instead of node-temp
This change switches the use of the node-temp module to our
@atom/temp fork so that we can benefit from a newer rimraf
version that will resolve some recurring test failures on
AppVeyor.

Resolves #1203.
@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Mar 7, 2018

Member

Ok, removing the cleanupSync calls seems to have fixed the issue. @daviwil you think that's an acceptable solution?

Member

50Wliu commented Mar 7, 2018

Ok, removing the cleanupSync calls seems to have fixed the issue. @daviwil you think that's an acceptable solution?

@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil

daviwil Mar 7, 2018

Member

Potentially acceptable to get the AppVeyor tests running again, but we definitely need to fix the underlying issue because it's had user impact for a long time according to some really old bugs I saw. Looks like you're making progress on the watchPath conversion, I'd be interested to see if that actually fixes the core problem enough to get the tests passing :)

Thanks again for taking this on!

Member

daviwil commented Mar 7, 2018

Potentially acceptable to get the AppVeyor tests running again, but we definitely need to fix the underlying issue because it's had user impact for a long time according to some really old bugs I saw. Looks like you're making progress on the watchPath conversion, I'd be interested to see if that actually fixes the core problem enough to get the tests passing :)

Thanks again for taking this on!

@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil

daviwil Mar 22, 2018

Member

Thanks for the quick fix @50Wliu! Let's go with this for now to get the build green and then finish up the watchPath migration later.

Member

daviwil commented Mar 22, 2018

Thanks for the quick fix @50Wliu! Let's go with this for now to get the build green and then finish up the watchPath migration later.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Mar 22, 2018

Member

/cc @atom/maintainers:

This PR contains a "quick-fix" to get Appveyor tests passing again. It does so by 1) switching over to @atom/temp (which may or may not be required) and 2) removing the manual cleanup step after each test.

Downsides of this PR is that at least on Windows, automatic cleanup does not occur for the GUI-based spec runner, leaving many files in the temp directory. Realistically, this should not be a concern as temp directories are cleaned regularly in the first place, and because there is no preserved state on CI.

A more complete fix which addresses the root cause (node-pathwatcher aggressively locking directories and preventing them from being removed) is in progress at #1232.

Member

50Wliu commented Mar 22, 2018

/cc @atom/maintainers:

This PR contains a "quick-fix" to get Appveyor tests passing again. It does so by 1) switching over to @atom/temp (which may or may not be required) and 2) removing the manual cleanup step after each test.

Downsides of this PR is that at least on Windows, automatic cleanup does not occur for the GUI-based spec runner, leaving many files in the temp directory. Realistically, this should not be a concern as temp directories are cleaned regularly in the first place, and because there is no preserved state on CI.

A more complete fix which addresses the root cause (node-pathwatcher aggressively locking directories and preventing them from being removed) is in progress at #1232.

@50Wliu 50Wliu merged commit 890157e into master Mar 22, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@50Wliu 50Wliu deleted the dw-use-atom-temp branch Mar 22, 2018

@damieng

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng Mar 22, 2018

Contributor

Ship it! :)

Contributor

damieng commented Mar 22, 2018

Ship it! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment