RFR: 7901757: Race in counting total number of failures from TestNG

[Pavel Rappo]

On Fri, 26 Jul 2024 09:52:40 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

> Can I please get a review of this change which proposes to fix the issue noted in https://bugs.openjdk.org/browse/CODETOOLS-7901757?
> 
> As noted there, the current implementation in jtreg's testng test listener doesn't take into account that the callback methods on that listener instance can be invoked concurrently for different threads. As a result, the count tracking logic in the listener ends up in a race condition which results in reporting incorrect numbers.
> 
> The commit in this PR fixes that issue and introduces a self test which reproduces the race issue and verifies the fix. The new test and existing tests continue to pass with this change.

I'm not a Reviewer in this project, but I'm the submitter of the original bug. Thanks for finally addressing it. From my perspective, the approach looks correct and straightforward.

src/share/classes/com/sun/javatest/regtest/agent/TestNGRunner.java line 121:

> 119: 
> 120:         private final AtomicInteger count = new AtomicInteger();
> 121:         private final AtomicInteger successCount  = new AtomicInteger();

Suggestion:

        private final AtomicInteger successCount = new AtomicInteger();

-------------

PR Review: https://git.openjdk.org/jtreg/pull/216#pullrequestreview-2201466704
PR Review Comment: https://git.openjdk.org/jtreg/pull/216#discussion_r1692839091