Skip to content
This repository has been archived by the owner on Dec 23, 2023. It is now read-only.

Commit

Permalink
Fix flaky LinkTest (#2082)
Browse files Browse the repository at this point in the history
LinkTest has recently been having flaky failures. It turns out that the
`link_ToString()` test relied on hash ordering. The failing assertion
only passed when a copy of the `HashMap` had the same `toString()` as
the original map. If copying the map changed the iteration order, the
test would fail.

This entire test function is questionable, however, as it is testing the
`toString()` method on an `AutoValue`. This is problematic, because
`AutoValue` specifies its `toString()` as "returning a useful (but
unspecified) string representation of the instance" (see
https://github.com/google/auto/blob/master/value/userguide/index.md#whats-going-on-here).
As such, it could make sense to remove this test altogether. This change
does not remove the test, because it's operating under the assumption
that the test is wanted.

This change fixes the test by checking that every entry in the map is
contained in the `link.toString()`. This means that the order of the
entries in the `link.toString()` is no longer important. Note that if
`Link` is changed to use a different map implementation not based on
`AbstractMap` that does not similarly guarantee its `toString()`, this
test may break, although the previous assert would also break in such a
situation.

Tested:
 - I tested this change by running LinkTest 100 times to verify that
   the flaky failure went away.
  • Loading branch information
Quincunx271 committed Nov 29, 2021
1 parent 4f217b4 commit 5f9a0fa
Showing 1 changed file with 9 additions and 2 deletions.
11 changes: 9 additions & 2 deletions api/src/test/java/io/opencensus/trace/LinkTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,18 @@ public void link_ToString() {
assertThat(link.toString()).contains(spanContext.getTraceId().toString());
assertThat(link.toString()).contains(spanContext.getSpanId().toString());
assertThat(link.toString()).contains("CHILD_LINKED_SPAN");
assertThat(link.toString()).contains(attributesMap.toString());
for (Map.Entry<String, AttributeValue> entry : attributesMap.entrySet()) {
// This depends on HashMap#toString(), via AbstractMap#toString(), having a specified format.
// In particular, each entry is formatted as `key=value`, with no spaces around the `=`.
// If Link is changed to use something other than a HashMap, this may no longer pass.
assertThat(link.toString()).contains(entry.getKey() + "=" + entry.getValue());
}
link = Link.fromSpanContext(spanContext, Type.PARENT_LINKED_SPAN, attributesMap);
assertThat(link.toString()).contains(spanContext.getTraceId().toString());
assertThat(link.toString()).contains(spanContext.getSpanId().toString());
assertThat(link.toString()).contains("PARENT_LINKED_SPAN");
assertThat(link.toString()).contains(attributesMap.toString());
for (Map.Entry<String, AttributeValue> entry : attributesMap.entrySet()) {
assertThat(link.toString()).contains(entry.getKey() + "=" + entry.getValue());
}
}
}

0 comments on commit 5f9a0fa

Please sign in to comment.