Skip to content

Reject @BeforeMethod in Multi-threaded tests#15914

Merged
highker merged 1 commit intoprestodb:masterfrom
v-jizhang:before_after_method
May 25, 2021
Merged

Reject @BeforeMethod in Multi-threaded tests#15914
highker merged 1 commit intoprestodb:masterfrom
v-jizhang:before_after_method

Conversation

@v-jizhang
Copy link
Contributor

@v-jizhang v-jizhang commented Apr 7, 2021

This replaces #15756.

Existence of @BeforeMethod or @AfterMethod most often indicates a
shared mutable state that needs to be e.g. reset. This means the class
should be marked @test(singleThreaded=true)
Also allow @BeforeMethod and @AfterMethod on non-single-threaded test
classes when the whole suite is marked as single-threaded.

Depends on https://github.com/facebookexternal/presto-facebook/pull/1543

== RELEASE NOTES ==
General Changes
* Reject @BeforeMethod in Multi-threaded tests.

@aweisberg
Copy link
Contributor

Is part of this backported from Trino? Need to add the co-author tag and reference the original PR.

@v-jizhang
Copy link
Contributor Author

Is part of this backported from Trino? Need to add the co-author tag and reference the original PR.

Yes, I'll add

@v-jizhang v-jizhang force-pushed the before_after_method branch from 5179e6e to d4fe85c Compare April 7, 2021 18:29
Copy link
Contributor

@aweisberg aweisberg left a comment

Choose a reason for hiding this comment

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

Why did no tests violate the rule this time? Why does presto-elasticsearch not use this? Can you link to the original PR?

See my comment, I think this won't work as intended, but not sure. We registered our listeners in presto-main.

Copy link
Contributor

@aweisberg aweisberg Apr 9, 2021

Choose a reason for hiding this comment

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

We don't use this file here? https://github.com/prestodb/presto/blob/master/presto-main/src/test/resources/META-INF/services/org.testng.ITestNGListener

I don't think this actually works as intended, but not 100% sure. I suspect only the unit test actually incorporates this listener. The rest won't be using it.

If we are going to do presto-testing-services we should move everything over to it and make sure it's being leveraged by all the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There will be cyclic dependencies if put the listeners under presto-main so the listeners must reside in a separate project.
I debugged into the listeners and id really works. I will consolidate all the listeners into one place.

@v-jizhang
Copy link
Contributor Author

Backported from trinodb/trino#5412 and trinodb/trino#5420

@v-jizhang
Copy link
Contributor Author

v-jizhang commented Apr 13, 2021

Why did no tests violate the rule this time? Why does presto-elasticsearch not use this? Can you link to the original PR?

See my comment, I think this won't work as intended, but not sure. We registered our listeners in presto-main.

That's because it checks not only single-threaded class, but also single-threaded test suite and we don't have any violations.
There are some test failures in elasticsearch I need to resolve.

@v-jizhang v-jizhang force-pushed the before_after_method branch from 7b46027 to 4b429da Compare April 15, 2021 01:46
@v-jizhang v-jizhang requested a review from aweisberg April 15, 2021 15:26
@aweisberg aweisberg force-pushed the before_after_method branch from 9025286 to 85ec817 Compare May 3, 2021 14:32
@aweisberg
Copy link
Contributor

Force pushed to run FB integration test, wasn't getting a result before.

@aweisberg
Copy link
Contributor

Hmm after my rebasing it doesn't build anymore.

@aweisberg aweisberg force-pushed the before_after_method branch 2 times, most recently from a34b767 to 1cd67be Compare May 13, 2021 14:16
Copy link
Contributor

@aweisberg aweisberg left a comment

Choose a reason for hiding this comment

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

Waiting for a clean test run

@aweisberg aweisberg force-pushed the before_after_method branch 2 times, most recently from b183519 to 660732b Compare May 19, 2021 16:36
@aweisberg aweisberg requested a review from highker May 19, 2021 16:39
@aweisberg
Copy link
Contributor

Had a clean run, but this wasn't squashed so I squashed and rebased and am waiting for a clean run again.

@aweisberg aweisberg self-requested a review May 19, 2021 19:32
@aweisberg aweisberg force-pushed the before_after_method branch 7 times, most recently from d17fc19 to 8884f41 Compare May 24, 2021 14:00
Existence of @BeforeMethod or @AfterMethod most often indicates a
shared mutable state that needs to be e.g. reset. This means the class
should be marked @test(singleThreaded=true)
Also allow @BeforeMethod and @AfterMethod on non-single-threaded test
classes when the whole suite is marked as single-threaded.

Co-authored-by: Piotr Findeisen <findepi@users.noreply.github.com>
@aweisberg aweisberg force-pushed the before_after_method branch from 8884f41 to f5bb3b2 Compare May 24, 2021 14:01
@aweisberg aweisberg force-pushed the before_after_method branch from 8884f41 to f5bb3b2 Compare May 24, 2021 14:01
Copy link
Contributor

@aweisberg aweisberg left a comment

Choose a reason for hiding this comment

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

It finally works! @highker Quick please review before someone adds another test that fails on @BeforeMethod ;-)

@highker highker merged commit 56c5e52 into prestodb:master May 25, 2021
@jainxrohit jainxrohit mentioned this pull request Jun 5, 2021
4 tasks
@ajaygeorge ajaygeorge mentioned this pull request Jun 9, 2021
4 tasks
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