Assertions refactoring #1580
Assertions refactoring #1580
Conversation
Method names improvement.
refactor with custom assertj assertions
Minor refactorings, mostly renaming. Fixed typos.
| maps.assertContainsAllEntriesOf(info, dataPointAttributes, attributesMap); | ||
| } | ||
| }); | ||
| public final MetricAssert hasDataPointsWithOneAttribute(AttributeMatcher expectedAttribute) { |
There was a problem hiding this comment.
[for reviewer] I left this method for convenience, because there are lots of assertions with just one attribute, but I can remove it if it makes matching logic harder to understand.
| .hasDataPointsWithAttributes( | ||
| attributeSet( | ||
| attribute("state", "replication"), | ||
| attributeWithAnyValue("region_server")), |
There was a problem hiding this comment.
[for reviewer] Thanks to new attribute assertion logic I discovered that in original implementation region_server was not checked!
| public String getName() { | ||
| return name; | ||
| } |
There was a problem hiding this comment.
[minor] add javadoc that it returns the name of the attribute
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) { | ||
| return true; | ||
| } | ||
| if (!(o instanceof AttributeMatcher)) { | ||
| return false; | ||
| } | ||
| AttributeMatcher other = (AttributeMatcher) o; | ||
| return Objects.equals(name, other.name) | ||
| && attributeValueMatcher.matchValue(other.attributeValueMatcher); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| // Do not use value matcher here to support value wildcards | ||
| return Objects.hash(name); | ||
| } |
There was a problem hiding this comment.
this breaks the usual equals/hashcode contract, which is generally not a good idea as it does not preserve the "logical equality". If I understand we have two use-cases:
- store it in a map for lookup by attribute name.
- check if attribute is matching or not.
For the lookup, we should store it in a map with a call to getName as key, and for the attribute matching it would be simpler to have a public boolean matches(String name, String value) and inline the AttributeValueMatcher which is almost only doing a String.equals(...).
There was a problem hiding this comment.
I believe equals/hashcode contract is not broken here sice two equal objects will always have the same hashcode. However I agree it's a bit tricky use of equals/hashcode, mostly to get Set<> equality working out of the box to support verification if specific set of attributes is matched by set of matchers. I'll think about cleaner solution to avoid confusion.
There was a problem hiding this comment.
Refactoring is done. So now code is explicitly calling matchesValue method instead of using equals/hashcode tandem behind the scene
| public class DataPointAttributes { | ||
| private DataPointAttributes() {} | ||
|
|
||
| public static AttributeMatcher attribute(String name, String value) { |
There was a problem hiding this comment.
[minor] add javadoc to explain the differences between all those methods: exact match, match with any value (even if the method name is quite clear on the latter, but not on the first on the exact match here).
| for (NumberDataPoint dataPoint : dataPoints) { | ||
| Map<String, String> map = toMap(dataPoint.getAttributesList()); | ||
|
|
||
| Set<AttributeMatcher> dataPointAttributes = toSet(dataPoint.getAttributesList()); |
There was a problem hiding this comment.
as suggested in the comment in AttributeMatcher this relies on the equals/hashcode implementation here, keeping a map would allow to avoid relying on this. We could probably add an extra check in the toMap implementation to ensure that there aren't any duplicate entries if there is an error in the test code.
Also, here we are using the AttributeMatcher class to carry the attributes key/values which is definitely not a "matcher".
| int matchCount = 0; | ||
| for (int i = 0; i < attributeSets.length; i++) { | ||
| if (mapEquals(map, attributeSets[i])) { | ||
| if (attributeSets[i].equals(dataPointAttributes)) { |
There was a problem hiding this comment.
here the call to equals should be replaced by calling the AttributeMatcher.matches(String name, String value) method.
There was a problem hiding this comment.
Not so simple, it is a comparison of collections.
| attributeSet( | ||
| attributeWithAnyValue("context"), attributeWithAnyValue("id")))) |
There was a problem hiding this comment.
[for reviewer] in this case the values are random so we could not assert them with exact values.
There was a problem hiding this comment.
This seems good to me. It might be nice to try and keep the terminology a bit closer to what exists in the core sdk-testing repo -- like hasAttributesSatisfyingExactly() and hasAttributesSatisfying() etc. In fact, I also wonder if it doesn't make sense to contribute some of this up there and then leverage it here?
...rc/integrationTest/java/io/opentelemetry/contrib/jmxscraper/assertions/AttributeMatcher.java
Outdated
Show resolved
Hide resolved
| this.attributeName = attributeName; | ||
| this.attributeValue = null; |
There was a problem hiding this comment.
Constructors should call each other:
| this.attributeName = attributeName; | |
| this.attributeValue = null; | |
| this(attributeName, null); |
| boolean matchesValue(String value) { | ||
| if ((attributeValue == null) || (value == null)) { | ||
| return true; | ||
| } | ||
| return Objects.equals(attributeValue, value); | ||
| } |
There was a problem hiding this comment.
Hmmmm, this looks like this would be true:
new AttributeMatcher('foo', 'bar').matchesValue(null)
Is that right? Intentional?
There was a problem hiding this comment.
Good catch! This is leftover from the previous approach that I missed to update.
Definitely yes. I think we can do that when we have completed the "groovy to yaml" migration in this repository and we start to work on (maybe reuse them as-is) the metrics definitions from instrumentation. The nice thing by doing this here first is that it's easier for us to experiment on what is the right approach that fits the existing definitions. |
…jmxscraper/assertions/AttributeMatcher.java Co-authored-by: jason plumb <[email protected]>
…emetry-java-contrib into assertions-refactoring
Assertions refactoring
New way of performing data point attributes assertions.
AttributeMatcher and AttributeValueMatcher classes provides possibility to verify attributes basing only on attribute name (if we do not know what attribute value should be, like hostname) and basing on name and value.
Few tests are updated. More will come in subsequent PRs.
Part of #1573