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

CA: Run all table test subtests in parallel #7452

Merged
merged 2 commits into from
Apr 25, 2024
Merged

Conversation

pgporada
Copy link
Member

Previously I had made a change to run all CA tests in parallel, which was great, but I failed to account for several table driven tests. By rebinding the subtest's iterator to the lexical scope, each subtest can now run in parallel.

@pgporada pgporada requested a review from a team as a code owner April 24, 2024 14:23
@aarongable
Copy link
Contributor

Given go1.22's change to how loopvars work, this rebinding shouldn't be necessary. These loops should Just Work (tm) and be able to run in parallel anyway.

@pgporada
Copy link
Member Author

pgporada commented Apr 24, 2024

I would love for that to be the case, but I receive warnings like the one reported here.

Edit: This report is more apt golang/go#66876

aarongable
aarongable previously approved these changes Apr 24, 2024
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

Ah, good find. So the rebindings are necessary not to appease the Go compiler, but to appease a bug in gopls. In that case, this LGTM, but let's file a bug to remove all foo := foo rebindings once gopls gets an update to fix the bug.

ca/ca_test.go Outdated Show resolved Hide resolved
ca/ca_test.go Outdated Show resolved Hide resolved
@pgporada pgporada merged commit 5cc3210 into main Apr 25, 2024
12 checks passed
@pgporada pgporada deleted the more-parallel-tests branch April 25, 2024 17:34
vbaranovskiy-plesk pushed a commit to plesk/boulder that referenced this pull request May 30, 2024
[Previously](letsencrypt#7438) I had
made a change to run all CA tests in parallel, which was great, but I
failed to account for several table driven tests. By rebinding the
subtest's iterator to the lexical scope, each subtest can now run in
parallel.
AlinaADmi pushed a commit to plesk/boulder that referenced this pull request Jul 29, 2024
[Previously](letsencrypt#7438) I had
made a change to run all CA tests in parallel, which was great, but I
failed to account for several table driven tests. By rebinding the
subtest's iterator to the lexical scope, each subtest can now run in
parallel.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants