Skip to content

Fix race conditions in invoker_test - #6905

Merged
cmbrose merged 9 commits into
trunkfrom
cmbrose/flaky-invoker-tests
Jan 25, 2023
Merged

Fix race conditions in invoker_test#6905
cmbrose merged 9 commits into
trunkfrom
cmbrose/flaky-invoker-tests

Conversation

@cmbrose

@cmbrose cmbrose commented Jan 23, 2023

Copy link
Copy Markdown
Member

Called out by @mislav over here the invoker tests don't properly wait for the listener to spin up, or for the grpc server to spin down. This creates some races in the invoker tests where new listeners may fail to bind as previous tests haven't fully cleaned up their goroutines.

@cmbrose
cmbrose requested a review from a team as a code owner January 23, 2023 20:30
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Jan 23, 2023
@cliAutomation

Copy link
Copy Markdown
Contributor

Hi! Thanks for the pull request. Please ensure that this change is linked to an issue by mentioning an issue number in the description of the pull request. If this pull request would close the issue, please put the word 'Fixes' before the issue number somewhere in the pull request body. If this is a tiny change like fixing a typo, feel free to ignore this message.

Comment thread internal/codespaces/rpc/test/server.go
Comment thread internal/codespaces/rpc/test/server.go Outdated
@jungaretti

Copy link
Copy Markdown
Contributor

I have a couple of questions about your changes, but they are mainly for my own learning. Lifting listener initialization to the test makes sense to me.

Comment thread internal/codespaces/rpc/invoker_test.go Outdated
Comment thread internal/codespaces/rpc/invoker_test.go Outdated
Comment thread internal/codespaces/rpc/test/server.go
Comment thread internal/codespaces/rpc/test/server.go Outdated
Comment thread internal/codespaces/rpc/invoker_test.go Outdated
@mislav

mislav commented Jan 24, 2023

Copy link
Copy Markdown
Contributor

Note: the Projectsv2 flakey test is being fixed in #6909

@mislav mislav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great. Thanks for working on this!

Please merge at your own discretion, but feel free to squash-merge for history simplification.

Comment thread internal/codespaces/rpc/invoker_test.go Outdated
@cmbrose
cmbrose merged commit f669a10 into trunk Jan 25, 2023
@cmbrose
cmbrose deleted the cmbrose/flaky-invoker-tests branch January 25, 2023 20:57
@jungaretti

Copy link
Copy Markdown
Contributor

I noticed something interesting while testing this branch in a codespace: port 16634 is always unavailable because the "real" gRPC server running on my codespace uses it. We should use a different port in a followup PR.

@mislav

mislav commented Jan 26, 2023

Copy link
Copy Markdown
Contributor

@cmbrose Thanks for this! Do you think this PR also addresses the intermittent failure that happened for the 2nd-to-last commit in this branch?

--- FAIL: TestRebuildContainerFailure (0.00s)
    invoker_test.go:256: error connecting to internal server: error connecting to internal server: failed to share remote port 16634: dial tcp 127.0.0.1:16634: connect: connection refused
FAIL
FAIL	github.com/cli/cli/v2/internal/codespaces/rpc	0.054s

@cmbrose

cmbrose commented Jan 26, 2023

Copy link
Copy Markdown
Member Author

@mislav it should - the very last commit is meant to fix it, although the commit message doesn't really explain that 😅 Definitely let me know if they start popping up again though!

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

Labels

external pull request originating outside of the CLI core team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants