-
Notifications
You must be signed in to change notification settings - Fork 323
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
fix failing Nextjs tests on Node versions < 18 #3970
fix failing Nextjs tests on Node versions < 18 #3970
Conversation
Overall package sizeSelf size: 5.84 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3970 +/- ##
==========================================
+ Coverage 85.00% 85.03% +0.02%
==========================================
Files 238 238
Lines 10194 10194
Branches 33 33
==========================================
+ Hits 8665 8668 +3
+ Misses 1529 1526 -3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
const { rawExpectedSchema } = require('./naming') | ||
|
||
const BUILD_COMMAND = NODE_MAJOR < 18 | ||
? 'yarn exec next build' : 'NODE_OPTIONS=--openssl-legacy-provider yarn exec next build' | ||
const VERSIONS_TO_TEST = NODE_MAJOR < 18 ? '>=11 <13.2' : '>=11' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>=11
or >=11.1
?
And, no need to set >=9.5 <11.1
?
https://github.com/DataDog/dd-trace-js/pull/3935/files#diff-dec1b363c4670835b0b6e96b3cebe6711e688d0dd14701a2cffaf6e6d1f60480L207-L208
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think >=9.5 <11.1 in the CI build was doing anything. I believe this comment mentions why < 11 is skipped in withVersions:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but in previous PR (sorry, I didn't realize it in the previous PR review), when the tracer <4
, they were executed for ['>=9.5 <11.1', '>=11.1 <13.2']
, because in with versions, when !DD_MAJOR >= 4
range is false and it reads environment variable to fill it.
a69f9d1
to
7fb85e5
Compare
BenchmarksBenchmark execution time: 2024-01-16 18:34:50 Comparing candidate commit 7fb85e5 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 259 metrics, 7 unstable metrics. |
* fix failing Nextjs tests on Node versions < 18 * address feedback
* fix failing Nextjs tests on Node versions < 18 * address feedback
* fix failing Nextjs tests on Node versions < 18 * address feedback
* fix failing Nextjs tests on Node versions < 18 * address feedback
* fix failing Nextjs tests on Node versions < 18 * address feedback
* fix failing Nextjs tests on Node versions < 18 * address feedback
* fix failing Nextjs tests on Node versions < 18 * address feedback
What does this PR do?
Adds explicit handling of Next js tests on different Node versions
Motivation
Next js tests failing on release branches after updating the Nextjs CI build in this PR: #3935