Skip to content

Conti/all tooling changes - #7023

Closed
wconti27 wants to merge 80 commits into
masterfrom
conti/all-tooling-changes
Closed

Conti/all tooling changes#7023
wconti27 wants to merge 80 commits into
masterfrom
conti/all-tooling-changes

Conversation

@wconti27

@wconti27 wconti27 commented Dec 3, 2025

Copy link
Copy Markdown
Contributor

What does this PR do?

Summary of Changes:

  • added assertion helper that allows for asserting type matching for assertObjectContains (useful for asserting error tags since stacktrace / error message is difficult to get exact values)
  • added abstraction layer for plugin tests. The abstraction handles withVersions setup, mock agent setup, tracer initialization, and mock agent closing. Allows plugin test files to be simpler, less to generate for an AI Agent.

Motivation

Plugin Checklist

Additional Notes

@wconti27 wconti27 self-assigned this Dec 12, 2025
@datadog-datadog-prod-us1

This comment has been minimized.

@pr-commenter

pr-commenter Bot commented Dec 12, 2025

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-01-06 16:12:54

Comparing candidate commit bcdd9ba in PR branch conti/all-tooling-changes with baseline commit 2681a5e in branch master.

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

Comment thread packages/dd-trace/src/plugins/tracing.js Outdated
Comment thread .gitignore Outdated
Comment thread packages/dd-trace/src/plugins/consumer.js Outdated
rochdev
rochdev previously approved these changes Dec 22, 2025
@wconti27
wconti27 marked this pull request as ready for review December 22, 2025 21:00
@wconti27
wconti27 requested a review from a team as a code owner December 22, 2025 21:00
Comment thread integration-tests/helpers/index.js
Comment thread packages/dd-trace/src/plugins/composite.js Outdated
Comment thread packages/dd-trace/test/setup/helpers/integration-test-helpers/index.js Outdated
for (const [name, PluginClass] of Object.entries(this.constructor.plugins)) {
this[name] = new PluginClass(...args)
// Handle case where value is an array of plugin classes
if (Array.isArray(PluginClass)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we already have cases where this would be used?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, but the idea would be for us to still be able to use .assertSomeTrace({span object}), mainly for tags that we don't know the exact value, such as error stack traces.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +28 to +30
const ANY_STRING = Symbol.for('test.ANY_STRING')
const ANY_NUMBER = Symbol.for('test.ANY_NUMBER')
const ANY_VALUE = Symbol.for('test.ANY_VALUE')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we already use these somewhere in our code where it would make the assertion more readable?

I think it is best to not only add a tool but to use it right away to justify its addition.

Should we just export these instead of using Symbol.for? That way we guarantee that there is no typo while using them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added it to exports 👍

Comment thread integration-tests/helpers/index.js Outdated

const assertObjectContains = function assertObjectContains (actual, expected, msg) {
// If expected contains asymmetric matchers, use our custom implementation
if (containsAsymmetricMatchers(expected)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are we expecting to use the asymmetric matchers in most situations? Depending on that, I would use a different implementation: Instead of having to iterate through the object as a whole each time, we could just use a try / catch. If an error is thrown, we rerun the assertion using the asymmetric matchers. If that throws as well, we throw the original caught error. That way the average case stays fast and there is no need to check anything in addition (because we expect no asymmetric matchers being used).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Really they are mainly for targeting scenarios, especially error tests since we can't know stacktrace reliably but should test for it. Or any other tags where the value may change between tests. Okay, I made that change 👍

@wconti27
wconti27 requested a review from BridgeAR January 5, 2026 18:03
@wconti27 wconti27 closed this Jan 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants