Skip to content

Update startup log tests to account for updated Node.js behavior#6513

Merged
bm1549 merged 7 commits intomainfrom
brian.marks/simplify-startup-log-test
Mar 20, 2026
Merged

Update startup log tests to account for updated Node.js behavior#6513
bm1549 merged 7 commits intomainfrom
brian.marks/simplify-startup-log-test

Conversation

@bm1549
Copy link
Copy Markdown
Contributor

@bm1549 bm1549 commented Mar 17, 2026

Motivation

Changes

  • Removes the trace flush from Node.js
  • Updates the manifest for Node.js default behavior for v6

Workflow

  1. ⚠️ Create your PR as draft ⚠️
  2. Work on you PR until the CI passes
  3. Mark it as ready for review
    • Test logic is modified? -> Get a review from RFC owner.
    • Framework is modified, or non obvious usage of it -> get a review from R&P team

🚀 Once your PR is reviewed and the CI green, you can merge it!

🛟 #apm-shared-testing 🛟

Reviewer checklist

  • Anything but tests/ or manifests/ is modified ? I have the approval from R&P team
  • A docker base image is modified?
    • the relevant build-XXX-image label is present
  • A scenario is added, removed or renamed?

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 17, 2026

CODEOWNERS have been resolved as:

manifests/nodejs.yml                                                    @DataDog/dd-trace-js
tests/parametric/test_startup_logs.py                                   @DataDog/system-tests-core @DataDog/apm-sdk-capabilities

@bm1549 bm1549 changed the title System tests no longer require Node.js specific handling Update startup log tests to account for updated Node.js behavior Mar 18, 2026
@bm1549 bm1549 marked this pull request as ready for review March 19, 2026 22:45
@bm1549 bm1549 requested review from a team as code owners March 19, 2026 22:45
@bm1549 bm1549 requested review from anna-git and removed request for a team March 19, 2026 22:45
@simon-id
Copy link
Copy Markdown
Member

Did you run the test locally to make sure the test passes ? The CI is not going to run the test you activated right now because the declaration is >=6.0.0 and the tested dev version is 6.0.0-pre, and in npm semver, prerelease versions do not match with non prerelease versions.
Just saying this test will not be run at all until we release v6, then it might start failing out of nowhere and break CI for everyone in system tests.

@bm1549
Copy link
Copy Markdown
Contributor Author

bm1549 commented Mar 20, 2026

Did you run the test locally to make sure the test passes ? The CI is not going to run the test you activated right now because the declaration is >=6.0.0 and the tested dev version is 6.0.0-pre, and in npm semver, prerelease versions do not match with non prerelease versions. Just saying this test will not be run at all until we release v6, then it might start failing out of nowhere and break CI for everyone in system tests.

@simon-id - yep, just confirmed it xpass'd locally for dd-trace-js v6

@simon-id
Copy link
Copy Markdown
Member

@bm1549 that's good, but since that doesn't add it to the CI, the risk of it starting to silently fail after you merge this, and start failing the whole system test CI once we release v6 is suboptimal

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@bm1549
Copy link
Copy Markdown
Contributor Author

bm1549 commented Mar 20, 2026

@simon-id misinterpreted your prior feedback - just added updated the ref in the manifest so it runs on 6.x

@datadog-official
Copy link
Copy Markdown

datadog-official bot commented Mar 20, 2026

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 50dd378 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback!

Copy link
Copy Markdown
Collaborator

@nccatoni nccatoni left a comment

Choose a reason for hiding this comment

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

Detail: I would suggest using the last failing version with > (something like >5.90.0) or the current pre-release version directly so it is more readable

@simon-id
Copy link
Copy Markdown
Member

@nccatoni this isn't possible actually, prerelease versions cannot be compared with non-prerelease versions, and this change will only apply when v6.0.0 comes out, we don't know when

@nccatoni
Copy link
Copy Markdown
Collaborator

nccatoni commented Mar 20, 2026

@simon-id - I'm not sure what you mean by that. Pre release versions can be compared to any other version without problem otherwise the manifests wouldn't work. We follow the semver standard for version comparison. You can checkout the ruby, go or php manifests for exemples if you want

bm1549 and others added 2 commits March 20, 2026 11:38
Change from >=6.0.0-0 to >5.90.0 which is equivalent (includes all
6.0.0 prereleases) but more readable per nccatoni's review suggestion.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nknown

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@simon-id
Copy link
Copy Markdown
Member

@nccatoni ah i just tried locally and our implementation does match what you're saying, it's the npm implementation that is different which is why i always get confused with prerelease stuff. The semver standard used to be ambiguous about prerelease comparison, which looks like it's been fixed in the latest draft of the spec.

With our ST custom implem:

range = SemverRange('>=6.0.0.pre')
>>> semver.Version('7.0.0-pre') in range
True

In node.js:

> require('semver').satisfies('6.0.0', '>=6.0.0.pre')
false

So we can use >=6.0.0-pre like you said, unless I'm missing another detail.

(btw your idea of using >5.90.0 is not possible because we will never know which version of the v5.x branch will be the final one, as we operate on multiple release lines at the same time)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@simon-id
Copy link
Copy Markdown
Member

LGTM but once CI passes, double check it's correctly running the test in dev, but not in prod 👍

@bm1549
Copy link
Copy Markdown
Contributor Author

bm1549 commented Mar 20, 2026

@simon-id just double checked. Merging now

Thanks for all the feedback!

@bm1549 bm1549 merged commit f00b5a4 into main Mar 20, 2026
448 checks passed
@bm1549 bm1549 deleted the brian.marks/simplify-startup-log-test branch March 20, 2026 17:19
annacai21 pushed a commit that referenced this pull request Mar 24, 2026
Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
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