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

Don't listen for arguments from new processes in test or benchmarks #19455

Merged
merged 2 commits into from Jun 6, 2019

Conversation

Projects
None yet
2 participants
@as-cii
Copy link
Member

commented Jun 6, 2019

This pull request prevents the following macOS firewall prompt from showing up when running tests:

Screen Shot 2019-06-06 at 10 24 59

The prompt was being shown because we had some code within listenForArgumentsFromNewProcess that would conditionally create a domain socket file when AtomApplication wasn't being instantiated in test or benchmark environments. For test/benchmark environments, this meant that this.socketPath would be undefined.

Unfortunately, the conditional didn't extend all the way down until listening for incoming connections, which had the unfortunate side effect of calling server.listen with an undefined argument (this.socketPath). In turn, this would instruct Node to listen for any TCP connection on a random port, which would then kick off the firewall given that the machine could potentially accept connections coming from the Internet or LAN.

With this pull request, we are changing that behavior by moving the conditional up, and only call listenForArgumentsFromNewProcess when instantiating a real AtomApplication.

@as-cii as-cii requested review from rafeca and nathansobo Jun 6, 2019

@as-cii as-cii force-pushed the as/fix-firewall-prompt-in-tests branch from 33f3240 to 67e101d Jun 6, 2019

@rafeca

rafeca approved these changes Jun 6, 2019

Copy link
Contributor

left a comment

Ups! 😅Thanks for fixing this!

@as-cii as-cii merged commit 119fead into master Jun 6, 2019

1 of 2 checks passed

Atom Pull Requests #20190606.5 failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@as-cii as-cii deleted the as/fix-firewall-prompt-in-tests branch Jun 6, 2019

@as-cii

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

I am still not sure why autocomplete-plus tests are failing, I am investigating that right now. They seem to fail consistently after #19446 got merged, but I am not really sure how that pull request could cause those tests to fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.