ci: add randomized matrix for better test coverage

[vlsi]

See https://github.com/vlsi/github-actions-random-matrix

Motivation and Context

This enables testing with different Java distributions, Java versions, Locales, Time Zones, etc.

Open issues so far:

• [ ] https://github.com/spockframework/spock/issues/1414 (reproducer PR https://github.com/spockframework/spock/pull/1415)
• [ ] JMeter seems to rely on hashCode uniqueness same hashcode jobs fail with

ERROR: unexpected output for BUG_62847.csv:
  - expected 247 bytes, /Users/runner/work/jmeter/jmeter/bin/testfiles/BUG_62847.csv
  + actual 86 bytes, /Users/runner/work/jmeter/jmeter/bin/BUG_62847.csv
@@ -1,7 +1,2 @@
 label,responseCode,responseMessage,threadName,success
-SuccessLoop,200,OK,TG 1-1,true
-DS_after_loop,200,OK,TG 1-1,true
-SuccessWhile,200,OK,TG 2-1,true
-DS_after_while,200,OK,TG 2-1,true
-Success_FEC,200,OK,TG 3-1,true
-DS_after_fec,200,OK,TG 3-1,true
+DS_after_fec,200,OK,TG 1-1,true
ERROR: unexpected output for BUG_62847.xml:
  - expected 480 bytes, /Users/runner/work/jmeter/jmeter/bin/testfiles/BUG_62847.xml
  + actual 149 bytes, /Users/runner/work/jmeter/jmeter/bin/BUG_62847.xml
@@ -1,10 +1,5 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <testResults version="1.2">
-<sample s="true" lb="SuccessLoop" rc="200" rm="OK" tn="TG 1-1"/>
-<sample s="true" lb="DS_after_loop" rc="200" rm="OK" tn="TG 1-1"/>
-<sample s="true" lb="SuccessWhile" rc="200" rm="OK" tn="TG 2-1"/>
-<sample s="true" lb="DS_after_while" rc="200" rm="OK" tn="TG 2-1"/>
-<sample s="true" lb="Success_FEC" rc="200" rm="OK" tn="TG 3-1"/>
-<sample s="true" lb="DS_after_fec" rc="200" rm="OK" tn="TG 3-1"/>
+<sample s="true" lb="DS_after_fec" rc="200" rm="OK" tn="TG 1-1"/>

[codecov-commenter]

Codecov Report

Merging #693 (f97d21a) into master (af7fc4d) will increase coverage by 0.00%. The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #693   +/-   ##
=========================================
  Coverage     55.59%   55.59%           
  Complexity    10336    10336           
=========================================
  Files          1059     1059           
  Lines         65045    65045           
  Branches       7399     7399           
=========================================
+ Hits          36160    36161    +1     
  Misses        26336    26336           
+ Partials       2549     2548    -1     

Impacted Files	Coverage Δ
...n/java/org/apache/jmeter/reporters/Summariser.java	90.07% <0.00%> (-0.77%)	:arrow_down:
...a/org/apache/jmeter/timers/PoissonRandomTimer.java	78.37% <0.00%> (+5.40%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update af7fc4d...f97d21a. Read the comment docs.

[vlsi]

The move from HashMap to IdentityHashMap in HashTree and ListedHashTree breaks two tests that check the behavior of HashTree.equals:

FAILURE   0,0sec, org.apache.jorphan.collections.PackageTest > testEqualsAndHashCode1()
    java.lang.AssertionError
        at org.junit.Assert.fail(Assert.java:87)
        at org.junit.Assert.assertTrue(Assert.java:42)
        at org.junit.Assert.assertTrue(Assert.java:53)
        at org.apache.jorphan.collections.PackageTest.testEqualsAndHashCode1(PackageTest.java:56)

org.apache.jorphan.collections.PackageTest > testEqualsAndHashCode2() failure marker
FAILURE   0,0sec, org.apache.jorphan.collections.PackageTest > testEqualsAndHashCode2()
    java.lang.AssertionError
        at org.junit.Assert.fail(Assert.java:87)
        at org.junit.Assert.assertTrue(Assert.java:42)
        at org.junit.Assert.assertTrue(Assert.java:53)
        at org.apache.jorphan.collections.PackageTest.testEqualsAndHashCode2(PackageTest.java:116)

The reason for test failure is that IdentityHashMap.equals uses reference equality for both keys and values, so data.put(key, new HashTree()); in the default HashTree constructor generates "non-equal" IdentityHashMap objects since new HashTree() is different every time.

I'm inclined the test should be corrected: remove the offending .equals comparison.

@pmouawad , @FSchumacher , any thoughts? The root cause of the problem is that TestElement instances have no identity (e.g. they have no primary key), so they should not be put to HashMap keys. Unfortunately HashTree (and ListedHashTree) used to put test elements as keys of HashMap instances, which might produce wrong results when importing .jmx. testfiles/BUG_62847.xml is impacted by the bug if there's a hashcode collision.

[vlsi]

For now, I implemented custom equals and hashCode for HashTree and ListedHashTree so they use reference equality for keys and object equality for values which turns out to be consistent with https://github.com/apache/jmeter/commit/1524b85d23f90f6394079420ec42885e110521b9

[vlsi]

I'm inclined to merge this and check how it works.

I think I've polished most of the glitches.

[FSchumacher]

Looks good (as always :)) I have a few questions, though.

• Has the usage of the synchronized identitymap a major performance impact and should we try to get it back to something concurrent?
• Is the copyright by you line in matrix_build.js file intentionally?

[vlsi]

synchronized identitymap a major performance impact and should we try to get it back to something concurrent?

I've no idea. We already had synchronized maps, so it should not be much worse.

An alternative option would be changing AbstractTestElement.equals to this==that comparison, and add contentEquals(AbstractTestElement other) method. It might be somewhat surprising if clients used equals, however, then we won't need to use IdentityHashMap all over the place.

WDYT?

[vlsi]

matrix_build.js is taken from https://github.com/vlsi/github-actions-random-matrix, so the copyright is retained. I believe it is worth copying the script, so it does not require cloning extra repository when CI builds the matrix. At the same time, it is easier to test the matrix locally by running node marrix.js (you don't need to retrieve matrix_build.js)

[vlsi]

@undera do you have any opinion on org.apache.jmeter.testelement.AbstractTestElement#equals ?

Currently AbstractTestElement#hashCode uses identityHashCode, so it was supposed to use reference equality when being a part of hashMap. However, AbstractTestElement#equals used propMap.equal(other.propMap), so if someone accidentally puts TestElement in a HashMap, then they could get wrong results if hashCode values accidentally collide.

In this PR, I tried replacing the corresponding HashMap usages in JMeter with IdentityHashMap. It turns out it required many changes. The "good" part is that it keeps backward compatibility. However, all the users would have to figure out and perform the same replacements.

There's another possibility: change AbstractTestElement#equals to final equals(Object o) { return this == o; }. Then it would automatically support AbstractTestElement in HashMap and ConcurrentHashMap. At the same time we could add AbstractTestElement#contentEquals for those who need to compare contents of the AbstractTestElement instead of their identity.

It looks like making AbstractTestElement#equals to compare object identity (this==that) would break backward compatibility (e.g. if someone used .equals to compare test element contents), however, it would automatically support TestElements in Set<...>, Map<....

WDYT?

[undera]

I never dug into this part of the code. From looking at the changes made, I'm a bit scared and confused by this change. This problem looks complex to understand, maybe you can draft a piece of code demonstrating the issue.

The base concept behind equals() and hashCode() should remain compliant to the original meaning from Java, to not cause strange effects. AbstractTestElement.equals() treats elements as equal based on their properties, including NAME, so it makes perfect sense, with exception of the TestElement's class mismatch possible (Assertion can match the Timer). Different TestElements would interpret the same set of properties differently, hence not equal. Let's consider code below.

    @Test
    public void testDemoProblem() throws Exception {
        AbstractTestElement o1 = new CompareAssertion();
        AbstractTestElement o2 = new ConstantTimer();
        Assert.assertEquals(o1.hashCode(), o2.hashCode()); // this one fails, makes sense
        Assert.assertEquals(true, o1.equals(o2)); // problem here?
    }

From the example above, we have a behavior that violates the idea of hashCode()+equal() from https://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#hashCode()

Overall, I don't quite understand the issue and how real is it. I'd ask for a code piece to reproduce and illustrate the issue.

Maybe the problem is that makeProperty() is able to generate entries that are equal from different objects, but that's outside of this method's responsibility. I would state that it's on caller's responsibility to change the NAME or some other attribute of the resulting property, to make it distinguishable. Current caller just puts hashCode() into NAME, which is guaranteed to produce duplicates. Looks like the CollectionProperty would be the main place to look at, maybe the distinct name should be set inside it.

That's it. Not sure how my comments help :)

[vlsi]

Here's a problem:

    @Test
    public void testDemoProblem() throws Exception {
        AbstractTestElement o1 = new ConstantTimer();
        AbstractTestElement o2 = new ConstantTimer();
        Set<Timer> timers = new HashSet<>();
        timers.add(o1);
        timers.add(o2);
        Assert.assertEquals(2, timers.size(), "there should be 2 timers in the set");
    }

Pretty much the same issue happens in SearchByClass which is used to search ThreadGroup, ResultCollector, etc elements. It is expected that SearchByClass would find all instances, however, if hash codes collide, then SearchByClass would produce fewer elements than present in the test plan.

Pretty much the same happens if you load .jmx file.

For instance, try doing the following:

1. Run JMeter with -XX:+UnlockExperimentalVMOptions -XX:hashCode=2

$ export _JAVA_OPTIONS="-XX:+UnlockExperimentalVMOptions -XX:hashCode=2"
$ ./gradlew runGui

2. Then open bin/testfiles/BUG_62847.jmx, and save it as bin/testfiles/BUG_62847_2.jmx

You'll see that JMeter discards two of three thread groups. In other words, I would expect that the file should be pretty much the same after re-save, however, the file is significantly different.

[undera]

I took the code snippet you suggested and it works fine for me (on master branch), just as expected, because the hashCode() is different. IMO the example with HashSet behaves exactly how it should. It's a Set that is supposed to store only objects that are different by hashCode().

Can you arrange the code piece to demonstrate the issue with SearchByClass? I don't see in its source code any relation to equals() or hashCode(). The criteria for search there is searchClass.isAssignableFrom()

Also, if I open bin/testfiles/BUG_62847.jmx and re-save it with master branch code, it works fine. Let's understand the problem better and let's reproduce it on the level of unit tests.

For now, the only change I see needed is to make sure that test elements of different kind won't be matched.

[vlsi]

because the hashCode() is different.

Why are you so sure hashCode is always different? There are at most 2**32 different hashCode values, so sometimes hashCode will collide.

If you launch OpenJDK with -XX:+UnlockExperimentalVMOptions -XX:hashCode=2, then the default Object#hashCode would return 1.

[vlsi]

Also, if I open bin/testfiles/BUG_62847.jmx and re-save it with master branch code

Did you perform export _JAVA_OPTIONS="-XX:+UnlockExperimentalVMOptions -XX:hashCode=2" before running "master branch code"?

[undera]

Did you perform export _JAVA_OPTIONS="-XX:+UnlockExperimentalVMOptions -XX:hashCode=2" before running "master branch code"?

No I did not. I don't understand what that option does and I stated above my full trust in standard approach around equals() and hashCode().

[undera]

Why are you so sure hashCode is always different? There are at most 2**32 different hashCode values, so sometimes hashCode will collide.

I went through your code piece under the debugger and I saw that the hashCode for two objects were different. The statement about 1/(2**32) event as sufficiently frequent does not sound to me. It's a very low probability and that's why Java relies on this result type of hashCode.

My point here is that I don't see any realistic evidence of a problem and struggling to understand it.

[vlsi]

The statement about 1/(2**32) event as sufficiently frequent does not sound to me

According to the birthday problem, the collision would happen with probability of 50% at 2**16 elements, which is not that uncommon. Then, if something goes wrong, it would be extremely hard to identify: JMeter would skip certain samplers or configuration elements, so it is more like a silent data corruption rather than "crash once every 2**32 JMeter executions".

[undera]

I'm not sure where this discussion go to. Do we discuss Java's hashCode being unreliable? I thought it's about some issue inside JMeter, though that issue does not show up in current master. I'm totally confused here.

[vlsi]

Sample failure in the current master:

public class SearchByClassTest {
    @Test
    public void test() {
        ListedHashTree tree = new ListedHashTree();
        int count = 100000;
        for (int i = 0; i < count; i++) {
            tree.add(new ThreadGroup());
        }
        SearchByClass<AbstractThreadGroup> searcher = new SearchByClass<>(AbstractThreadGroup.class);
        tree.traverse(searcher);
        Assertions.assertEquals(count, searcher.getSearchResults().size());
    }
}

org.opentest4j.AssertionFailedError: expected: <100000> but was: <99998>
	at app//org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at app//org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at app//org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
	at app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:145)
	at app//org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:527)
	at app//org.apache.jorphan.collections.SearchByClassTest.test(SearchByClassTest.java:35)

[vlsi]

@FSchumacher , @pmouawad , @undera do you have any preference for the solution?

Just to remind: currently AbstractTestElement has mismatching equals and hashCode (see https://github.com/apache/jmeter/pull/693#issuecomment-1301792127) which breaks code like SearchByClass that might find fewer elements than it should (see sample in https://github.com/apache/jmeter/pull/693#issuecomment-1304755910)

I have two possible solutions:

1. Correct AbstractTestElement#hashCode to match AbstractTestElement#equals, so the compare contents (values in properties) rather that object identity. It would require many changes when TestElement is a key in HashMap. For instance, SearchByClass collects elements in a HashSet, and it would merge equal test elements, so we'll have to replace HashMap with IdentityHashMap in SearchByClass and similar places. See the current PR for the set of changes.

2. Correct AbstractTestElement#equals to match AbstractTestElement#hashCode, so they use compare identity, and they always treat different objects unequal. It would automatically support cases when TestElement is placed in HashMap key, however, a new contentEquals method would be required to compare test element contents (e.g. when searching an argument. See https://github.com/apache/jmeter/pull/5727 for the set of changes.

Unfortunately, both approaches will break backward compatibility one way or another. I do not think it is a severe breakage though, so I believe it is fine to have either of them in the upcoming 5.6.

I suggest going with approach 2, so we introduce contentEquals and contentHashCode methods. It seems a slightly smaller change overall, and it makes test elements safer to store in maps and sets. TestElement is mutable, so it would be slightly better if its hashCode and equals did not change as TestElement mutates.

However, I checked the sources of jmeter-plugins, jmeter-java-dsl, jmeter-maven-plugin and none of them seems to be impacted by this change (they do not store TestElement in HashMap, and they do not seem to use TestElement#equals)

WDYT?

[undera]

To me, the option #2 looks more logical.

[FSchumacher]

If we change the behaviour, I think #2 would be best, too.

[vlsi]

I rebased "option 2", and now I'm puzzled.

It fails on test cases like assertEquals(new Header("accept", "*/*"), httpSampler.getHeaderManager().getHeader(0)); https://github.com/apache/jmeter/blob/5020590c2dd6564e87cc2295e0090d2c6526c436/src/protocol/http/src/test/java/org/apache/jmeter/gui/action/ParseCurlCommandActionTest.java#L254-L255

The code that compares elements like if (file.equals(item)) { seems to be harder to spot. Then, it is not that often that test elements are placed in sets. Well, JMeter itself does that, however, it is unlikely plugins will deal with Map<TestElement.

Then, we won't be able to make TestElement#equal final as it might break compatibility with some of the plugins that happened to override the method for their own reasons.

So it might be a safer choice to go for "option 1", and make TestElement#hashCode and TestElement#equals compare element contents. Then all the places that need element identity (e.g. SearchByClass that needs to report individual elements even if their contents is the same) should use IdentityHashMap or something like that.

[vlsi]

I'm going to merge this PR shortly unless there are objections. An alternative https://github.com/apache/jmeter/pull/5727 is not dramatically smaller, and it breaks backward compatibility.

It looks like external users have much fewer usages for Map<TestElement, ...> than JMeter core, and TestElement#equals was probably used much more often for "comparing contents" in plugin code.

Unfortunately, the PR adds new usages of synchronizedMap, so we might need to replace them later. I believe they should not be dramatic