ci: add randomized matrix for better test coverage - #693
Conversation
40037be to
bcdbacb
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
b299a2f to
c8db52a
Compare
c8db52a to
f97d21a
Compare
f97d21a to
31d5940
Compare
1bca384 to
8068fac
Compare
8068fac to
bed24df
Compare
Use IdentityHashMap instead of HashMap when key is TestElement See apache#693
28ebcb6 to
4157d37
Compare
Use IdentityHashMap instead of HashMap when key is TestElement See apache#693
5ce41d5 to
5e4ed16
Compare
Use IdentityHashMap instead of HashMap when key is TestElement See apache#693
5e4ed16 to
1ce3bb4
Compare
Use IdentityHashMap instead of HashMap when key is TestElement See apache#693
1ce3bb4 to
e9b867e
Compare
Use IdentityHashMap instead of HashMap when key is TestElement See apache#693
e9b867e to
db8984f
Compare
Use IdentityHashMap instead of HashMap when key is TestElement See apache#693
7125112 to
d5eb569
Compare
Use IdentityHashMap instead of HashMap when key is TestElement See apache#693
|
@undera do you have any opinion on org.apache.jmeter.testelement.AbstractTestElement#equals ? Currently In this PR, I tried replacing the corresponding There's another possibility: change It looks like making WDYT? |
|
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 @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 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 That's it. Not sure how my comments help :) |
|
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 Pretty much the same happens if you load For instance, try doing the following:
$ export _JAVA_OPTIONS="-XX:+UnlockExperimentalVMOptions -XX:hashCode=2"
$ ./gradlew runGui
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. |
|
I took the code snippet you suggested and it works fine for me (on master branch), just as expected, because the Can you arrange the code piece to demonstrate the issue with Also, if I open For now, the only change I see needed is to make sure that test elements of different kind won't be matched. |
Why are you so sure If you launch OpenJDK with |
Did you perform |
No I did not. I don't understand what that option does and I stated above my full trust in standard approach around |
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. |
According to the birthday problem, the collision would happen with probability of 50% at |
|
I'm not sure where this discussion go to. Do we discuss Java's |
|
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());
}
} |
|
@FSchumacher , @pmouawad , @undera do you have any preference for the solution? Just to remind: currently I have two possible solutions:
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 However, I checked the sources of WDYT? |
|
To me, the option #2 looks more logical. |
|
If we change the behaviour, I think #2 would be best, too. |
…Map instead of HashMap when key is TestElement See apache#693
|
I rebased "option 2", and now I'm puzzled. It fails on test cases like The code that compares elements like Then, we won't be able to make So it might be a safer choice to go for "option 1", and make |
…Map instead of HashMap when key is TestElement See apache#693
|
I'm going to merge this PR shortly unless there are objections. It looks like external users have much fewer usages for Unfortunately, the PR adds new usages of |
__changeCase uses java-default locale, so the test does not support tr_TR locale. See apache#5723
…Map instead of HashMap when key is TestElement See apache#693
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:
Spock does not work in tr_TR locale: Unrecognized block label: given @ line 31, column 13. spockframework/spock#1414 (reproducer PR ci: add randomized test matrix for better test coverage spockframework/spock#1415)
Support ParallelExecutionConfigurationStrategy with Turkish locale junit-team/junit-framework#2890
Avoid relying on item.hashCode() for Map and Collection properties #5923
CSVDataSet might produce wrong results in SHARE_GROUP or SHARE_THREAD modes as it relies on System.identityHashCode uniqueness for ThreadGroup and Thread #5925
same hashcodejobs fail with. Fixed by movingHashTreetoIdentityHashMapand movingMap<TestElement, ..toIdentityHashMapas wellJava 8 + same hashcode fails at
:src:dist:processSiteXsltwithVerifyError. I'm inclined to skipprocessSiteXsltwhen building with Java8 and same hashcode. We might retry if the bug is solved when xalan is updated.