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.

Sign up for GitHub

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

Jump to bottom

cmd/geth: fixed parallelized tests #22002

Merged
merged 1 commit into from
Dec 14, 2020
Merged

cmd/geth: fixed parallelized tests #22002

merged 1 commit into from
Dec 14, 2020

Conversation

MariusVanDerWijden
Copy link
Member

Two tests in our codebase use the t.Parallel() in an unsafe way which results in only the last test in a test table being executed.
For example master prouduces:

=== RUN   TestDeliverHeadersHang
=== PAUSE TestDeliverHeadersHang
=== CONT  TestDeliverHeadersHang
=== RUN   TestDeliverHeadersHang/protocol_63_mode_full
=== PAUSE TestDeliverHeadersHang/protocol_63_mode_full
=== RUN   TestDeliverHeadersHang/protocol_63_mode_fast
=== PAUSE TestDeliverHeadersHang/protocol_63_mode_fast
=== RUN   TestDeliverHeadersHang/protocol_64_mode_full
=== PAUSE TestDeliverHeadersHang/protocol_64_mode_full
=== RUN   TestDeliverHeadersHang/protocol_64_mode_fast
=== PAUSE TestDeliverHeadersHang/protocol_64_mode_fast
=== RUN   TestDeliverHeadersHang/protocol_64_mode_light
=== PAUSE TestDeliverHeadersHang/protocol_64_mode_light
=== RUN   TestDeliverHeadersHang/protocol_65_mode_full
=== PAUSE TestDeliverHeadersHang/protocol_65_mode_full
=== RUN   TestDeliverHeadersHang/protocol_65_mode_fast
=== PAUSE TestDeliverHeadersHang/protocol_65_mode_fast
=== RUN   TestDeliverHeadersHang/protocol_65_mode_light
=== PAUSE TestDeliverHeadersHang/protocol_65_mode_light
=== CONT  TestDeliverHeadersHang/protocol_63_mode_full
65=== CONT  TestDeliverHeadersHang/protocol_65_mode_full
65=== CONT  TestDeliverHeadersHang/protocol_65_mode_light
65=== CONT  TestDeliverHeadersHang/protocol_64_mode_fast
65=== CONT  TestDeliverHeadersHang/protocol_64_mode_light
65=== CONT  TestDeliverHeadersHang/protocol_65_mode_fast
65=== CONT  TestDeliverHeadersHang/protocol_64_mode_full
65=== CONT  TestDeliverHeadersHang/protocol_63_mode_fast
65

while this branch produces:

=== RUN   TestDeliverHeadersHang
=== PAUSE TestDeliverHeadersHang
=== CONT  TestDeliverHeadersHang
=== RUN   TestDeliverHeadersHang/protocol_63_mode_full
=== PAUSE TestDeliverHeadersHang/protocol_63_mode_full
=== RUN   TestDeliverHeadersHang/protocol_63_mode_fast
=== PAUSE TestDeliverHeadersHang/protocol_63_mode_fast
=== RUN   TestDeliverHeadersHang/protocol_64_mode_full
=== PAUSE TestDeliverHeadersHang/protocol_64_mode_full
=== RUN   TestDeliverHeadersHang/protocol_64_mode_fast
=== PAUSE TestDeliverHeadersHang/protocol_64_mode_fast
=== RUN   TestDeliverHeadersHang/protocol_64_mode_light
=== PAUSE TestDeliverHeadersHang/protocol_64_mode_light
=== RUN   TestDeliverHeadersHang/protocol_65_mode_full
=== PAUSE TestDeliverHeadersHang/protocol_65_mode_full
=== RUN   TestDeliverHeadersHang/protocol_65_mode_fast
=== PAUSE TestDeliverHeadersHang/protocol_65_mode_fast
=== RUN   TestDeliverHeadersHang/protocol_65_mode_light
=== PAUSE TestDeliverHeadersHang/protocol_65_mode_light
=== CONT  TestDeliverHeadersHang/protocol_63_mode_full
63=== CONT  TestDeliverHeadersHang/protocol_65_mode_light
65=== CONT  TestDeliverHeadersHang/protocol_64_mode_light
64=== CONT  TestDeliverHeadersHang/protocol_65_mode_fast
65=== CONT  TestDeliverHeadersHang/protocol_65_mode_full
65=== CONT  TestDeliverHeadersHang/protocol_64_mode_full
64=== CONT  TestDeliverHeadersHang/protocol_64_mode_fast
64=== CONT  TestDeliverHeadersHang/protocol_63_mode_fast
63

When printing out the tested protocol.

See also: https://gist.github.com/posener/92a55c4cd441fc5e5e85f27bca008721

holiman
holiman reviewed Dec 11, 2020
View reviewed changes
eth/downloader/downloader_test.go Outdated
@@ -1490,7 +1490,6 @@ func testFakedSyncProgress(t *testing.T, protocol int, mode SyncMode) {
// This test reproduces an issue where unexpected deliveries would
// block indefinitely if they arrived at the right time.
func TestDeliverHeadersHang(t *testing.T) {
t.Parallel()
Copy link
Contributor

Choose a reason for hiding this comment

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

You removed this -- was that intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, because we call t.Parallel() in the subtests already, calling it twice doesn't really make sense

Copy link
Contributor

@holiman holiman Dec 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, but those are different.
This one means

  • This test can be run in parallel (with other tests in this module)

The other one means

  • These subtests can be run in parallel (with eachother)

If you call Parallel twice on a t, it'll panic btw, so there's never any calling it twice

holiman
holiman reviewed Dec 11, 2020
View reviewed changes
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

Good catches, only one nitpick!

holiman
holiman approved these changes Dec 11, 2020
View reviewed changes
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM, let's merge after #21482

@holiman
Copy link
Contributor

holiman commented Dec 14, 2020

please rebase

@MariusVanDerWijden MariusVanDerWijden changed the title cmd/geth, eth/downloader: fixed parallelized tests cmd/geth: fixed parallelized tests Dec 14, 2020
@holiman holiman added this to the 1.10.0 milestone Dec 14, 2020
@holiman holiman merged commit 4859929 into ethereum:master Dec 14, 2020
@quorumbot quorumbot mentioned this pull request Sep 3, 2021
Merged
9 tasks
@MariusVanDerWijden MariusVanDerWijden deleted the fix_tests branch November 30, 2021 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@holiman holiman holiman approved these changes

@karalabe karalabe Awaiting requested review from karalabe

@rjl493456442 rjl493456442 Awaiting requested review from rjl493456442

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.10.0
Development

Successfully merging this pull request may close these issues.
2 participants
@MariusVanDerWijden @holiman