Respect setParent calls for noop subsegments. #215
Conversation
| public void testLeakedSubsegmentsAreCleanedBetweenInvocations() { | ||
|
|
||
| @SetEnvironmentVariable(key = "_X_AMZN_TRACE_ID", value = TRACE_HEADER_2) | ||
| void testNotSampledSetsParentToSubsegment() { |
There was a problem hiding this comment.
This is the new test, the rest are refactoring to replace powermock with junit5 extension
There was a problem hiding this comment.
Definitely a "side-by-side" instead of inline type of review :)
| public void testLeakedSubsegmentsAreCleanedBetweenInvocations() { | ||
|
|
||
| @SetEnvironmentVariable(key = "_X_AMZN_TRACE_ID", value = TRACE_HEADER_2) | ||
| void testNotSampledSetsParentToSubsegment() { |
There was a problem hiding this comment.
Definitely a "side-by-side" instead of inline type of review :)
| @@ -59,55 +58,71 @@ public void setupAWSXRay() { | |||
| } | |||
|
|
|||
| @Test | |||
| public void testBeginSubsegmentWithNullTraceHeaderEnvironmentVariableResultsInADummySegmentParent() { | |||
| testMockContext(TraceHeader.fromString(null), FacadeSegment.class); | |||
| void testBeginSubsegmentWithNullTraceHeaderEnvironmentVariableResultsInADummySegmentParent() { | |||
There was a problem hiding this comment.
I know this is mostly out of scope for the PR, but the name of this test isn't right. The name says we expect a Dummy (no-op) segment whereas what we test for/actually get is a facade segment.
I'm pretty sure contrary to the name of the test always using a facade segment is the behavior we want, and we can have a larger discussion if anyone on team disagrees, but for now renaming the test to avoid future confusion would be nice.
There was a problem hiding this comment.
Thanks - yeah good cleanup
| public void testBeginSubsegmentWithIncompleteTraceHeaderEnvironmentVariableResultsInADummySegmentParent() { | ||
| testMockContext(TraceHeader.fromString("a"), FacadeSegment.class); | ||
| @SetEnvironmentVariable(key = "_X_AMZN_TRACE_ID", value = "a") | ||
| void testBeginSubsegmentWithIncompleteTraceHeaderEnvironmentVariableResultsInADummySegmentParent() { |
Fixes #213
I mistook that the parent and parent segment are the same. The API is crufty in setting the parent after construction, even though it's a constant, that could be another improvement for a major version.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.