-
Notifications
You must be signed in to change notification settings - Fork 887
chore: Update pion/udp and improve parallel/non-parallel tests #7164
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
Conversation
After re-enabling port-forward tests, I noticed we've at some point ditched our fork of So I updated the package to the next version (last one before their v2 work). See: https://github.com/coder/coder/actions/runs/4723449296/jobs/8379423014?pr=7164 |
284114b
to
67caa6a
Compare
@@ -21,10 +21,8 @@ import ( | |||
"github.com/coder/coder/testutil" | |||
) | |||
|
|||
//nolint:tparallel,paralleltest // Subtests require setup that must not be done in parallel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hoping removing parallel from the parent will allow these to run more stable, so for now let's re-enable these tests.
err = <-errC | ||
require.ErrorIs(t, err, context.Canceled) | ||
}) | ||
t.Run(c.name+"_OnePort", func(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may also contribute to stability, one less test indentation. We probably need to start considering depth of tests and their interactions with each other or limiting depth to parent + subtest, not subsubtest.
Tests that mix parallel/non-parallel parent tests and subtests will behave unexpectedly, this PR attempts to clean some of these up and does a little restructuring.
Ultimately, don't mix parallel/non-parallel and avoid too much nesting (sub-subtests).