internal/exectracetest: move execution trace testing to separate module #2709
Conversation
BenchmarksBenchmark execution time: 2024-05-30 14:02:35 Comparing candidate commit 8f4e834 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 47 metrics, 0 unstable metrics. |
nsrip-dd
force-pushed
the
nick.ripley/only-finish-span-once
branch
from
297fed4
to
3c50bd6
Compare
To parse execution traces for testing execution trace-related functionality, we'll need to use golang.org/x/exp/trace. As new versions of the format come out, we'll need to upgrade that dependency to keep tests working. The golang.org/x/exp module is prone to breaking changes, so we should try not to inflict them on our users. Thus, this commit creates a separate module where execution trace testing logic can live, so we can freely upgrade golang.org/x/exp. Future commits will move existing test logic, which uses gotraceui's parser, to this new module.
nsrip-dd
force-pushed
the
nick.ripley/new-exectrace-test-module
branch
from
1f76dd1
to
10c4ad9
Compare
There was a problem hiding this comment.
Thanks for working on this 🙇 . I found a few problems.
Problem 1
The test passes even if I revert #2708.
$ git revert 3c50bd6f4f0856e10c54d419f691234a17867bc1
$ cd internal/exectracetest
$ go test .
ok gopkg.in/DataDog/dd-trace-go.v1/internal/exectracetest 0.458s
Problem 2
Moving the test into its own go module seems to prevent it from being executed by the CI according to the logs.
Problem 3
The golang.org/x/exp module is prone to breaking changes, so we should try not to inflict them on our users.
I don't follow. If we're only using x/exp from _test.go files, it should be impossible for x/exp changes to break our customers. The blast radius should strictly be limited to our test suite?
This was a sloppy mistake during a cleanup of the test case that failed to actually start the tracer. This made the test pass even when reverting the fix it was intended to test.
They're in a separate module so the first gotestsum command won't pick them up. We need to be working from the exectracetest directory.
There was a problem hiding this comment.
Thanks for the thorough review!
Problem 1 - test passes even after revert
😅 Fixed, I was sloppy and lost the bits to start & stop the APM tracer when tidying up the test case. Added back and confirmed that the test fails after reverting the fix it is meant to test.
Problem 2 - CI didn't run
Also good catch. I updated the CI script, should actually run the test now. You unfortunately won't see it here because PR tests run on Go 1.20 right now, which golang.org/x/exp doesn't support. But it will run on main, and we're planning to drop Go 1.20 support soon anyway (working on #2705) so soon it'll run on PRs, too.
Problem 3 - why a separate module
The issue is that there is only one entry for golang.org/x/exp in our main go.mod file. If we need a newer version, even if it's only for a test, our main go.mod file would need to be updated to specify at least that version. I've run into breakage in the past from upgrading golang.org/x/exp. See #2212 and my internal v1.55.0 release checklist for more, but basically golang.org/x/exp made a breaking change to some slice functions that broke the build internally.
What does this PR do?
Creates a new module for execution trace-related testing.
Motivation
To parse execution traces for testing execution trace-related
functionality, we'll need to use golang.org/x/exp/trace. As new versions
of the format come out, we'll need to upgrade that dependency to keep
tests working. The golang.org/x/exp module is prone to breaking changes,
so we should try not to inflict them on our users. Thus, this PR
creates a separate module where execution trace testing logic can live,
so we can freely upgrade golang.org/x/exp. Future PRs will move
existing test logic, which uses gotraceui's parser, to this new module.
Reviewer's Checklist
Unsure? Have a question? Request a review!