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

tracer: Fix TestMalformedTID so it passes on its own #2257

Merged
merged 4 commits into from
Oct 13, 2023

Conversation

evanj
Copy link
Contributor

@evanj evanj commented Oct 11, 2023

What does this PR do?

This test appears to be depending on previous tests because it does not pass when run on its own:

$ go test ./ddtrace/tracer -run=TestMalformedTID

--- FAIL: TestMalformedTID (0.01s)
    textmap_test.go:2079:
          Error Trace:  /Users/evan.jones/dd/dd-trace-go/ddtrace/tracer/textmap_test.go:2079
          Error:        Not equal:
                        expected: "640cfd8d00000000"
                        actual  : ""

Fix the test:

  • Call internal.SetGlobalTracer(tracer) which is required for the test to pass.
  • Pass t to assert functions instead of using a top-level assert object so the sub-tests fail correctly.
  • Do not set the headerPropagationStyleExtract environment variable. It does not seem to be required.

Additionally, while fixing this test I noticed similar problems in TestTracerStartSpanOptions128:

  • Create the assert object in each subtest so the correct test fails
  • defer internal.SetGlobalTracer(&internal.NoopTracer{}) to ensure this test does not accidentally affect other tests.

Motivation

I'm actually tracking down a race detector bug and noticed this test failing while setting up a new test.

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.

For Datadog employees:

  • If this PR touches code that handles credentials of any kind, such as Datadog API keys, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

This test appears to be depending on previous tests because it does
not pass when run on its own:

$ go test ./ddtrace/tracer -run=TestMalformedTID

--- FAIL: TestMalformedTID (0.01s)
    textmap_test.go:2079:
          Error Trace:  /Users/evan.jones/dd/dd-trace-go/ddtrace/tracer/textmap_test.go:2079
          Error:        Not equal:
                        expected: "640cfd8d00000000"
                        actual  : ""

Fix the test:
* Call internal.SetGlobalTracer(tracer) which is required for the
  test to pass.
* Create assert objects in the sub tests so the correct tests fail.
* Do not set the headerPropagationStyleExtract environment variable.
  It does not seem to be required.

Additionally, while fixing this test I noticed similar problems in
TestTracerStartSpanOptions128:
* Create the assert object in each subtest so the correct test fails
* defer internal.SetGlobalTracer(&internal.NoopTracer{}) to ensure
  this test does not accidentally affect other tests.
@evanj evanj requested a review from a team October 11, 2023 10:38
@pr-commenter
Copy link

pr-commenter bot commented Oct 11, 2023

Benchmarks

Benchmark execution time: 2023-10-11 17:56:02

Comparing candidate commit 658fdad in PR branch evan.jones/test-malformed-tid with baseline commit 7895436 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 40 metrics, 1 unstable metrics.

Copy link
Contributor

@ajgajg1134 ajgajg1134 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks!

@evanj evanj mentioned this pull request Oct 12, 2023
5 tasks
@felixge felixge enabled auto-merge (squash) October 13, 2023 11:24
@felixge felixge merged commit 26fb2f4 into main Oct 13, 2023
50 checks passed
@felixge felixge deleted the evan.jones/test-malformed-tid branch October 13, 2023 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants