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

["vlsi (via GitHub)" <gi...@apache.org>]

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

   @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?


-- 
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