[GitHub] [jmeter] undera commented on pull request #693: ci: add randomized matrix for better test coverage

[GitBox <gi...@apache.org>]

undera commented on PR #693:
URL: https://github.com/apache/jmeter/pull/693#issuecomment-1301962775

   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.
   ```java
       @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 :)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org