Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved keys assertions in ObservationContextAssert #3266

Merged

Conversation

simonbasle
Copy link
Contributor

The first part of this change adds two assertions around keys
(considering both low cardinality and high cardinality KeyValues).

It is currently possible to individually assert specific keys are
present, either focusing on the key itself being there or specifying the
expected key and value pair.
For example hasLowCardinalityKeyValueWithKey("k") or
hasHighCardinalityKeyValue("k", "v")). But it isn't possible to
quickly also make sure that no other keys are present outside of the
ones asserted via these methods.

Let's imagine my actual has 3 keys "A", "B" and "C" but I only expect
"A" and "B": without having prior knowledge of "C", its absence cannot
be asserted.

Adding hasKeyValuesCount is a good complement for this: in the example
having hasKeyValuesCount(2) would fail with a message indicating 3
keys are present instead of 2.

Adding hasOnlyKeys could also serve the same purpose if the only
concern is having the keys, whatever the values are. As such
hasOnlyKeys("A", "B") in the example would fail with a message saying
that extraneous key "C" was found. hasOnlyKeys("A", "B", "C", "D")
would also fail with a message stating that key "D" is missing.

The second part of this change fixes a counterintuitive behaviour of the
doesNotHave[High|Low]LevelKeyValue(String, String) assertions: if the
key is present with another value than the one specified, the assertion
currently fails by complaining the key is present.

But if both a key and a value are specified then only that specific
pair should be undesirable. So for that same key we should accept values
that do not match the one specified. (for a similar example in AssertJ
core, see Map's containsEntry assertion).

The first part of this change adds two assertions around keys
(considering both low cardinality and high cardinality KeyValues).

It is currently possible to individually assert specific keys are
present, either focusing on the key itself being there or specifying the
expected key and value pair.
For example `hasLowCardinalityKeyValueWithKey("k")` or
`hasHighCardinalityKeyValue("k", "v")`). But it isn't possible to
quickly also make sure that no other keys are present outside of the
ones asserted via these methods.

Let's imagine my `actual` has 3 keys "A", "B" and "C"  but I only expect
"A" and "B": without having prior knowledge of "C", its absence cannot
be asserted.

Adding `hasKeyValuesCount` is a good complement for this: in the example
having `hasKeyValuesCount(2)` would fail with a message indicating 3
keys are present instead of 2.

Adding `hasOnlyKeys` could also serve the same purpose if the only
concern is having the keys, whatever the values are. As such
`hasOnlyKeys("A", "B")` in the example would fail with a message saying
that extraneous key "C" was found. `hasOnlyKeys("A", "B", "C", "D")`
would also fail with a message stating that key "D" is missing.

The second part of this change fixes a counterintuitive behaviour of the
`doesNotHave[High|Low]LevelKeyValue(String, String)` assertions: if the
key is present with another value than the one specified, the assertion
currently fails by complaining the key is present.

But if both a key _and a value_ are specified then only that specific
pair should be undesirable. So for that same key we should accept values
that do not match the one specified. (for a similar example in AssertJ
core, see `Map`'s `containsEntry` assertion).
@simonbasle
Copy link
Contributor Author

feel free to split this PR in two if you feel the behavior change should be treated separately

@marcingrzejszczak marcingrzejszczak added the enhancement A general enhancement label Jul 6, 2022
@marcingrzejszczak marcingrzejszczak added this to the 1.10.0-M3 milestone Jul 6, 2022
@marcingrzejszczak marcingrzejszczak merged commit 512b1de into micrometer-metrics:main Jul 6, 2022
@simonbasle simonbasle deleted the improvedKeysAssertions branch July 6, 2022 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants