changes in behavior with assertEquals(Collection) et al

[stuart-marks]

TestNG Version

7.4.0

Expected behavior

assertEquals(Collection c1, Collection c2) is documented to compare the collections in the same order. It no longer does.

Actual behavior

assertEquals(Collection c1, Collection c2) now delegates to c1's equals() implementation via Objects.equals(), which can give different behavior behavior from earlier releases (e.g., 7.3.0).

Is the issue reproductible on runner?

• Shell
• Maven
• Gradle
• Ant
• Eclipse
• IntelliJ
• NetBeans

Test case sample

The following test fails on 7.3.0, as expected. However, it passes on 7.4.0 and on the testng-7.5.0-20210505.072953-25.jar snapshot, which is an unexpected change in behavior.

This was introduced by #2460, which changed a bunch of occurrences of actual == expected to Objects.equals(actual, expected). This resulted in silent changes of behavior. Some of this was fixed by #2487, which reverted the test back to actual == expected for assertSame and assertNotSame. I believe however that the changes in #2460 were ill-considered, as they potentially changed the behavior of several different cases. The example with assertEquals(Collection, Collection) is just one that I happened to notice. There are likely other behavior changes that resulted from #2460.

The #2460 change resulted in bugs in OpenJDK, among them https://bugs.openjdk.java.net/browse/JDK-8266780. This particular one was fixed by #2487. However, I'm concerned that even if we don't see other test failures, that we will now be testing something different from what we had expected to test when the test was written. This is a matter of great concern.

I note that the documentation for assertEquals(Collection, Collection) still says "Asserts that two collections contain the same elements in the same order" so the implementation now disagrees with the documentation.

import org.testng.annotations.Test;
import static org.testng.Assert.*;

import java.util.*;

@Test
public class AssertEqualsCollection {
    public void checkEquals() {
        Collection<String> set1 = new LinkedHashSet<>();
        Collection<String> set2 = new LinkedHashSet<>();

        set1.addAll(Arrays.asList("a", "b", "c"));
        set2.addAll(Arrays.asList("a", "c", "b"));

        assertEquals(set1, set2);
    }
}

[juherr]

@stuart-marks
Thanks for the report.

(... perhaps testng doesn't have enough testng tests ...)

It's disquieting that I was able to discern at least one other silent change in behavior after only a few minutes' inspection.

I agree with both statements: the test coverage is not as good as we can expect.
To be honest, the core team is too little and we are only adding new tests when a new issue is created, like yours.

Could you tell us what is the openjdk strategy about its test framework?
Can we expect some contributions from there?

[stuart-marks]

OK, it looks like #2545 reverted some (all?) of the problematic changes from #2460, so maybe this issue is settled now. But you did ask some larger questions that I should answer.

Could you tell us what is the openjdk strategy about its test framework?
Can we expect some contributions from there?

OpenJDK uses something called "jtreg" (JavaTest Regression framework). It's good at finding and reporting tests, and invoking fresh JVMs when necessary and reusing existing JVMs when possible. But it basically has no test API. Legacy jtreg tests simply have their main() invoked and either exit normally or throw an exception. More recently we've integrated a downstream fork of TestNG into jtreg. This enables us to write tests using TestNG's assertions, setup/teardown protocols, and things like data providers. Except for specialized cases, we're pretty much writing all new tests using TestNG-within-jtreg, and we'll occasionally convert an old main-based test to TestNG.

As such we are relying quite heavily on the exact behaviors of TestNG's assertions, so any change in existing behavior is of concern. The #2460 change did result in test failures where the test should have passed. But I think a greater concern is tests that should fail but that actually passes, because the change in the assertion permits looser behavior than it did before. The unordered/ordered distinction is an example of this. I don't know if an actual issue arose from this -- perhaps none did, because we caught it quickly. But the danger is that in the future, a regression is introduced. We'd expect tests to catch it... but if the test used an assertion whose behavior was changed to be looser, that regression might go undetected.

As for contributions, it's hard to say. I think the first thing is to start talking about stuff. We have a lot of expertise with specifications and exact behaviors of the language and the libraries, so maybe we could help out with reviews or discussions of potential future work. There are also some ideas floating around about enhancing the TestNG APIs to use the new records feature (Java 16) though maybe that's something for a bit farther down the road.

For collaboration, I'll look into joining the TestNG mailing list (or maybe have some folks from OpenJDK join). I could point you at some OpenJDK mailing lists as well, but there are a lot of them, and some of them are extremely high volume (and probably not of interest). But let me know and we can figure something out.

[stuart-marks]

As an aside, my colleague @mcimadamore has already filed an issue regarding records support; see #2241.

[vlsi]

@stuart-marks , hi there.
Do you think it is feasible to run testng test subset in cbeust/testng GitHub Actions to validate that upcoming TestNG releases do not break OpenJDK builds?
Does it sound like "build https://github.com/openjdk/jtreg with the latest testng", and "run the resulting testng over the latest openjdk"?
Do you have suggested jtreg commands (or references?)? (I have never actually ran jtreg yet)

Of course, it won't address hidden issues like "unexpected tests that should fail but that actually passes", however, running OpenJDK before TestNG release would definitely be an improvement.

[stuart-marks]

(oops, missed this)

I'm somewhat doubtful about whether OpenJDK can try out new versions of TestNG effectively. The jtreg thing is a mostly single-purpose framework which is only for testing the JDK. I doubt it's worth your time to try to build and run it. The integration work between jtreg and TestNG requires some coding work, I believe; it's more than a matter of dropping another jar file on the classpath. As a result we upgrade the TestNG version in jtreg only once a year or so. However, I'll ask about this to see if it's feasible.

My recommendation for the TestNG team is to think of the API "documentation" as more of a specification and to consider the unit tests for the TestNG APIs to be more like a conformance test suite, which tests conformance of the implementation against the specification. (This is of course colored by my work on Java and the JDK, which has a similar structure.) While there's only one implementation of TestNG, applying more rigor by thinking of things as specifications and conformance tests seems appropriate for something as fundamental as a test suite.