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

Fixes for Test_Connections in process_linux.go #1119

Merged
merged 4 commits into from Aug 28, 2021

Conversation

tbarker25
Copy link
Contributor

Test_Connections currently fails intermittently on Linux (and maybe other OSs).

On inspection, Go closes TCP connections when they go out of scope and are garbage collected. I've re-written Test_Connections() to avoid closing until the test has finished.

Other changes:

  • Close connections properly in TestConnections so running go test -times=N works
  • t.Skip() can not be called inside a test go-routine
  • Make sure that Test_AllProcesses_cmdLine doesn't ignore failures.
  • Some minor style changes motivated by staticcheck warnings.

@tbarker25 tbarker25 marked this pull request as ready for review August 17, 2021 19:52
@Lomanic
Copy link
Collaborator

Lomanic commented Aug 18, 2021

These changes also need to be applied in the v3 modules.

Commits history needs some love, the first one would be "Fix Test_Connections in process_linux.go" (only including changes to the Test_Connections function) with body the first part of your message in the PR and the proper connections closing bit, second commit would be about the various other fixes, with its body the rest of this PR message (or a separate commit for each point).

LGTM otherwise, tests are all green

@tbarker25 tbarker25 force-pushed the process-fixes-1 branch 2 times, most recently from 12dfbb6 to 79896a1 Compare August 19, 2021 14:31
@github-actions github-actions bot added the v3 label Aug 19, 2021
other OSs), and fails consistently if run with `go test -times=N`

On inspection, Go closes TCP connections when they go out of scope and
are garbage collected. I've re-written Test_Connections() to explicitly
close connectections once the test has finished. This has the other
benefit of closing gracefully, which means the -times argument should
work.

I've also removed the t.Skip() calls inside goroutines as they are
unsupported.
@tbarker25
Copy link
Contributor Author

These changes also need to be applied in the v3 modules.

Commits history needs some love, the first one would be "Fix Test_Connections in process_linux.go" (only including changes to the Test_Connections function) with body the first part of your message in the PR and the proper connections closing bit, second commit would be about the various other fixes, with its body the rest of this PR message (or a separate commit for each point).

LGTM otherwise, tests are all green

Thanks. I've made those changes.

@shirou
Copy link
Owner

shirou commented Aug 28, 2021

Sorry to late response. Thank you so much!

@shirou shirou merged commit 595a629 into shirou:master Aug 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants