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

abci/tests: stop abci server started in addr test #9093

Merged
merged 2 commits into from
Jul 27, 2022

Conversation

mark-rushakoff
Copy link
Contributor

This test would fail if run with "go test -count=2" because it uses a
fixed address and was not closing the server, so the subsequent run
could not bind to the address.

While closing the server is correct, it would probably be better if the
API was able to report the bound address so that we could pass
"localhost:0" for an anonymous port. But I am currently focusing on test
cleanup, not ready to change any existing APIs.

This test would fail if run with "go test -count=2" because it uses a
fixed address and was not closing the server, so the subsequent run
could not bind to the address.

While closing the server is correct, it would probably be better if the
API was able to report the bound address so that we could pass
"localhost:0" for an anonymous port. But I am currently focusing on test
cleanup, not ready to change any existing APIs.
@mark-rushakoff mark-rushakoff force-pushed the mr/test-add-missing-stop branch from de72e5f to 42787ef Compare July 25, 2022 19:02
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

Nice catch

@cmwaters cmwaters added the S:backport-to-v0.34.x Tell mergify to backport the PR to v0.34.x label Jul 26, 2022
Copy link
Contributor

@tychoish tychoish left a comment

Choose a reason for hiding this comment

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

we typically put the package name (somewhat loosely applied) rather than to functional area of the test in the commit message title, following the golang convention. This is somewhat different than the "conventional commit style".

@mark-rushakoff mark-rushakoff changed the title test: stop abci server started in addr test abci/tests: stop abci server started in addr test Jul 26, 2022
@mark-rushakoff
Copy link
Contributor Author

I updated the PR title to use abci/tests: as a prefix matching the affected package.

The PR is waiting on required statuses check-mocks and check-proto, maybe those aren't working right until after #9095 is finished/merged?

@mark-rushakoff mark-rushakoff merged commit c4e2352 into main Jul 27, 2022
@mark-rushakoff mark-rushakoff deleted the mr/test-add-missing-stop branch July 27, 2022 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S:backport-to-v0.34.x Tell mergify to backport the PR to v0.34.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants