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

Shared: Organize ganache cli script #780

Merged
merged 2 commits into from Apr 9, 2019

Conversation

Projects
None yet
4 participants
@facuspagnuolo
Copy link
Contributor

facuspagnuolo commented Apr 8, 2019

Main idea was to avoid relying on nc in order to kill a process that is running on the port we want to use. Instead I'm adding adding a cleanup function with trap to ensure we always end the rpc process we have started. If there is a foreign process running on that port we shouldn't kill it, it's expected to fail rather than taking over a port while killing the process that is using it.

It may fix #755 since we are not relying on nc anymore.

@facuspagnuolo facuspagnuolo requested review from bingen, sohkai and 0x6431346e Apr 8, 2019

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 8, 2019

Coverage Status

Coverage remained the same at 96.734% when pulling 3d6b9b4 on organize_ganache_cli_script into c4c936f on master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 8, 2019

Coverage Status

Coverage remained the same at 96.734% when pulling 9dd4b27 on organize_ganache_cli_script into c4c936f on master.

@sohkai

sohkai approved these changes Apr 9, 2019

Copy link
Member

sohkai left a comment

Awesome! Not too familiar with trap, but this seems really clean 😍

Show resolved Hide resolved shared/test-helpers/ganache-cli.sh Outdated
Show resolved Hide resolved shared/test-helpers/ganache-cli.sh Outdated
Show resolved Hide resolved shared/test-helpers/ganache-cli.sh Outdated

@facuspagnuolo facuspagnuolo merged commit f5dc8f8 into master Apr 9, 2019

5 checks passed

License Compliance All checks passed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 96.734%
Details
license/cla Contributor License Agreement is signed.
Details
@bingen
Copy link
Member

bingen left a comment

Nice job! Way cleaner this way!!

@@ -1,50 +1,64 @@
#!/usr/bin/env bash

# Exit script as soon as a command fails.
set -o errexit
set -o errexit -o pipefail

This comment has been minimized.

Copy link
@bingen

bingen Apr 11, 2019

Member

Not too familiar with this. Why do we need it? I don't see any pipe script.

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Apr 11, 2019

Author Contributor

True, it was instinctively

This comment has been minimized.

Copy link
@bingen

bingen Apr 12, 2019

Member

Haha, cool, I think it's ok to leave it anyway if you want (just in case), I just wanted to make sure I was not missing anything ;-)

}

if testrpc_running; then

This comment has been minimized.

Copy link
@bingen

bingen Apr 11, 2019

Member

Aren't we killing previous instances of testrpc anymore? Is this on purpose?

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Apr 11, 2019

Author Contributor

It was on purpose: the idea was to avoid relying on nc in order to kill a process that is running on the port we want to use. Instead I'm adding adding a cleanup function with trap to ensure we always end the rpc process we have started. If there is a foreign process running on that port we shouldn't kill it, it's expected to fail rather than taking over a port while killing the process that is using it.

This comment has been minimized.

Copy link
@bingen

bingen Apr 12, 2019

Member

Makes total sense!

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 12, 2019

Member

@facuspagnuolo How does ganache error if it can't bind to the requested port? If it's not very good, would it still make sense to test it before trying to run ganache on the same port so the error message is more human-readable?

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Apr 12, 2019

Author Contributor

I will fail with a classic EADDRINUSE error, see:

Ganache CLI v6.4.2 (ganache-core: 2.5.4)
listen EADDRINUSE 127.0.0.1:8545

Let me know if you'd prefer adding a custom error message

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 12, 2019

Member

That looks pretty good 😄

@facuspagnuolo facuspagnuolo deleted the organize_ganache_cli_script branch Apr 16, 2019

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.