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-unsubscr...@jmeter.apache.org

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

Reply via email to