-
Notifications
You must be signed in to change notification settings - Fork 273
Add BaseEngineParSeqTest.java to make test suite run in parallel with no race conditions #354
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
base: master
Are you sure you want to change the base?
Conversation
subprojects/parseq-test-api/src/main/java/com/linkedin/parseq/BaseEngineParSeqTest.java
Outdated
Show resolved
Hide resolved
| * | ||
| * @return ParSeqUnitTestHelper instance. | ||
| */ | ||
| protected static ParSeqUnitTestHelper getParSeqUnitTestHelper() { |
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.
Why expose this weirdly like this? Just make the instance protected?
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 just recalled why I made it static:
I am making it singleton with this logic because I need one instance for the whole test suite so that I can setup and teardown PAR_SEQ_UNIT_TEST_HELPER in beforeSuite and afterSuite methods respectively.
BeforeSuite and AfterSuite methods run before any test's constuctor (or its base class's constructor). So making PAR_SEQ_UNIT_TEST_HELPER non-static will cause NPE issues.
Regarding why BeforeSuite and AfterSuite methods, I already commented the reason from line 33 to 35.
| * com.linkedin.parseq.BaseEngineTest} executes setup()/teardown() at class level (which can cause | ||
| * race-conditions if two or more tests from the same class are executed in parallel). | ||
| * | ||
| * <p>2) It uses mutex locking mechanism to set up and tear down {@link ParSeqUnitTestHelper} to |
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.
Why is this static and not scoped per test class?
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.
Same as above, need a singleton instance of ParSeqUnitTestHelper
** Testing done **
Tested the code with
./gradlew clean build.Also tested with a giant private repo with thousands of unit tests (this base class also improved the test suite execution time by letting us run tests in parallel).
More details:
There are two changes in this PR:
Part 1:
We currently have
BaseEngineTest(which runssetup()andteardown()at method level) andBaseEngineParTest(which runssetup()andteardown()at class level) in parseq library. Both are prone to race condition issues forParseqUnitTestHelperinstance as it will be setup and teared down in different tests simultaneously when tests are executed in parallel.Added
BaseEngineParSeqTest.java, which runssetup()andtearDown()at test suite level and will also make sure that the sharedParseqUnitTestHelperinstance will not have any race condition issues. This will fix all the race condition issues and tests can be executed in parallel at suite level thereby reducing the test suite execution time.Execution hierarchy for unit test setup and teardown methods with @before* and @after* annotations:
@BeforeSuiteor@AfterSuite(executed only once per test suite)@BeforeClassor@AfterClass(executed for every test class)@BeforeMethodor@AfterMethod(executed for every test method)Part 2:
jcenteris sunset as of Aug 15 2024 (source: https://jfrog.com/blog/jcenter-sunset/). Also, we are using an older version ofbuild-info-extractor-gradle(the version doesn't exist in maven and gradle repository). Removing jcenter and updatingbuild-info-extractor-gradle(bare minimum changes to fix build issues).