facebook / buck

A fast build system that encourages the creation of small, reusable modules over a variety of platforms and languages.
https://buck.build
Apache License 2.0
8.56k stars 1.16k forks source link

[TestNG] Don't fail if TestNG runs in parallel #2591

Closed navkast closed 3 years ago

navkast commented 3 years ago

For TestNG, a @Test method can run concurrently using a @DataProvider:

@DataProvider(name = "testList", parallel = true)

This causes an exception in Buck as follows:

java.util.ConcurrentModificationException
    at java.base/java.util.ArrayList$Itr.checkForComodification(ArrayList.java:1042)
    at java.base/java.util.ArrayList$Itr.next(ArrayList.java:996)
    at com.facebook.buck.testrunner.BaseRunner.writeResult(BaseRunner.java:100)
    at com.facebook.buck.testrunner.TestNGRunner.run(TestNGRunner.java:96)
    at com.facebook.buck.testrunner.BaseRunner.runAndExit(BaseRunner.java:301)
    at com.facebook.buck.testrunner.TestNGMain.main(TestNGMain.java:48)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:566)
    at com.facebook.buck.jvm.java.runner.FileClassPathRunner.main(FileClassPathRunner.java:88)

The fix is to simply move the test runner's collection of test results to be a concurrency-safe collection. I use ConcurrentLinkedQueue: this should be faster than explicit synchronized blocks or using Collections.synchronizedList().

navkast commented 3 years ago

Test failure seems unrelated.

KapJI commented 3 years ago

Hi @navkast! Although it might seem unrelated, we still need the CI to be green before merging this. Try to rerun it.

KapJI commented 3 years ago

That test failure might be actually related.

navkast commented 3 years ago

Same test fails on another commit currently on master: https://app.circleci.com/pipelines/github/facebook/buck/2560/workflows/a7ebb86a-6503-4595-8ad4-065bdb738627/jobs/25291

KapJI commented 3 years ago

Sorry, can you bump it to 200ms? Hopefully this will help to make this test more reliable. https://github.com/facebook/buck/blob/ae8e8fc013413f0144b73971eceeff3fd6f51d6c/test/com/facebook/buck/util/concurrent/JobLimiterTest.java#L71

KapJI commented 3 years ago

LGTM, thanks!