test: replaced node-fetch with native fetch in test setup #2022

aryamohanan · 2025-09-22T11:36:45Z

Migrate test setup files from node-fetch-v2 to the native fetch API available in Node.js 18+. This change removes the dependency on node-fetch-v2 in the test code and uses the built-in fetch implementation instead.

Note:
This update replaces all usages of node-fetch-v2 in the test setup.

Background:

PR introducing node-fetch-v2: https://github.com/instana/nodejs/pull/1160/files
Reference: https://jsw.ibm.com/browse/INSTA-15850

kirrg001 · 2025-10-02T08:46:20Z

https://jsw.ibm.com/browse/INSTA-15850

aryamohanan · 2026-01-02T09:12:25Z

packages/collector/test/tracing/logging/console/test.js

-        'request to http://127.0.0.1:65212/ failed, reason: connect ' +
-          'ECONNREFUSED 127.0.0.1:65212 -- console.error - should be traced'
-      ));
+      runAndTrace('exit-span', true, 'fetch failed -- console.error - should be traced'));


Native fetch and node-fetch represent the same failure; they are just surfaced differently.

Native fetch:

Throws a TypeError: fetch failed

The real cause is nested under error.cause

node-fetch:

Throws a FetchError

The connection error is exposed directly

aryamohanan · 2026-01-06T07:37:12Z

packages/aws-lambda/test/lambdas/async.js

  if (process.env.DETACHED_REQUEST) {
    setTimeout(async () => {
-      await fetch(downstreamDummyUrl, { headers: { 'X-Downstream-Header': 'yes' } });
+      const fetchResponse = await fetch(downstreamDummyUrl, { headers: { 'X-Downstream-Header': 'yes' } });


This changes because native fetch and node-fetch v2 complete the request at different points in the lifecycle.

Native fetch resolves when response headers arrive, not when the body finishes. The response body is streamed, and if it isn’t consumed, the tracing span is finalized too early and no trace is emitted.

node-fetch v2 tracks the full HTTP lifecycle, so traces are generated even without reading the body.

Native fetch resolves when response headers arrive, not when the body finishes. The response body is streamed, and if it isn’t consumed, the tracing span is finalized too early and no trace is emitted.

Don't we have to add await response.json(); to all apps?

We usually only do

fetch(`http://127.0.0.1:${agentPort}/ping`) .then(() => { httpSuccess(res, data); })

No. I only applied this to the cases where tests were breaking (when fetch is called inside a setTimeout, etc.). In the other cases, using await fetch() or .then() allows the promise to resolve and the instrumentation’s .finally() to run, so the span is transmitted.

Discussed: this needs more investigation.

There is a bug/misbehavior in nativeFetch.

finally is triggered with a delay when using without .text() or .json(). This is not right IMO.

We already receive the response.headers in the then method.
I am quiet sure that the one who added finally intend to handle success or error in one place.

Similar to httpClient (please compare):

we wait for response headers

span.d + transmit is executed after the headers (not after the body)

Exactly, I’ll check this and see whether we can fix it.

Yeah we should fix this issue as a PR before this PR 👍

created a pr #2265

packages/collector/test/tracing/misc/stack_trace/test.js

abhilash-sivan · 2026-01-06T11:00:17Z

I believe this also needs to be removed!

nodejs/packages/collector/test/metrics/app/package.json

Line 21 in f1c571f

"node-fetch": "^2.6.0"

aryamohanan · 2026-01-07T03:58:05Z

I believe this also needs to be removed!

nodejs/packages/collector/test/metrics/app/package.json

Line 21 in f1c571f

"node-fetch": "^2.6.0"

@abhilash-sivan IMO, this is not part of the test setup code. It is a minimal test application (metrics-test-app) that exists solely to be test the metrics collection.

The purpose of the test is to verify that when this application runs with node-fetch as a dependency, the metrics system reports it correctly. Therefore, this dependency is not related to test setup and does not need to be replaced or removed the test explicitly requires an external dependency to validate the behavior.

abhilash-sivan

LGTM

kirrg001

See comments 👍

kirrg001 · 2026-01-07T20:54:10Z

packages/collector/test/tracing/misc/node-fetch/app.js

+// To use it in a CommonJS environment, we load it via a dynamic import and
+// forward all arguments to the default export.
+// eslint-disable-next-line no-shadow
+const fetch = (...args) => import('node-fetch').then(({ default: fetch }) => fetch(...args));


Why is this part of this PR? Looks like a separate PR to me.

I’ve created a separate PR to clarify and extend the tests: #2264.

kirrg001 · 2026-01-07T21:27:26Z

packages/aws-lambda/test/lambdas/async.js

  if (process.env.DETACHED_REQUEST) {
    setTimeout(async () => {
-      await fetch(downstreamDummyUrl, { headers: { 'X-Downstream-Header': 'yes' } });
+      const fetchResponse = await fetch(downstreamDummyUrl, { headers: { 'X-Downstream-Header': 'yes' } });


There is a bug/misbehavior in nativeFetch.

finally is triggered with a delay when using without .text() or .json(). This is not right IMO.

We already receive the response.headers in the then method.
I am quiet sure that the one who added finally intend to handle success or error in one place.

Similar to httpClient (please compare):

we wait for response headers

span.d + transmit is executed after the headers (not after the body)

sonarqubecloud · 2026-01-08T08:54:59Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

aryamohanan added the WIP label Sep 22, 2025

aryamohanan force-pushed the test-node-fetch branch from f6e2ca8 to 9a12921 Compare September 22, 2025 11:37

aryamohanan self-assigned this Sep 25, 2025

aryamohanan added the experimental label Sep 25, 2025

aryamohanan force-pushed the test-node-fetch branch 2 times, most recently from 8ae3d4d to 9b3dec7 Compare September 30, 2025 11:25

aryamohanan added the blocked label Oct 8, 2025

aryamohanan force-pushed the test-node-fetch branch from 9b3dec7 to 1c4034b Compare December 31, 2025 05:46

aryamohanan removed experimental blocked labels Dec 31, 2025

aryamohanan force-pushed the test-node-fetch branch 3 times, most recently from a2d3f22 to 72fbfc4 Compare January 1, 2026 09:40

aryamohanan commented Jan 2, 2026

View reviewed changes

aryamohanan marked this pull request as ready for review January 2, 2026 09:18

aryamohanan requested a review from a team as a code owner January 2, 2026 09:18

aryamohanan removed the WIP label Jan 2, 2026

aryamohanan added 12 commits January 5, 2026 15:47

test: replaced node-fetch with native fetch in test setup

2e3e78e

test: fargate test fix

89906a8

test: fix lambda

cbdc1f9

test: fix protocols

2360500

test: fix otel exporter and sampler

718e078

test: log fixed

a420431

test: console.log fix

f3c3afd

test: fix protocols

59dbe5a

test: skipped

85ab400

test: skipped

c3eb5c5

test: skipped

73a23d7

test: updated

d93342d

test: fix

f1ba9ae

aryamohanan force-pushed the test-node-fetch branch from b17ea69 to f1ba9ae Compare January 5, 2026 10:18

fix: misc

b55720e

aryamohanan commented Jan 6, 2026

View reviewed changes

aryamohanan added 4 commits January 6, 2026 13:50

test: cleanup

e8b71bd

test: cleanup

7849303

test: improved comments

fc0109f

test: comments

751c901

aryamohanan commented Jan 6, 2026

View reviewed changes

packages/collector/test/tracing/misc/stack_trace/test.js Show resolved Hide resolved

test: cleanup

f1c571f

chore: cleanup

3ac68d8

abhilash-sivan approved these changes Jan 7, 2026

View reviewed changes

kirrg001 assigned kirrg001 and unassigned aryamohanan Jan 7, 2026

kirrg001 added the do not merge label Jan 7, 2026

kirrg001 requested changes Jan 7, 2026

View reviewed changes

aryamohanan mentioned this pull request Jan 8, 2026

test: make node-fetch version handling explicit in tests #2264

Open

chore: review comment

3ed3a22

aryamohanan mentioned this pull request Jan 8, 2026

fix(native-fetch): transmitted span immediately after receiving headers #2265

Open

test: replaced node-fetch with native fetch in test setup #2022

Are you sure you want to change the base?

test: replaced node-fetch with native fetch in test setup #2022

Uh oh!

Conversation

aryamohanan commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kirrg001 commented Oct 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abhilash-sivan commented Jan 6, 2026

Uh oh!

aryamohanan commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abhilash-sivan left a comment

Choose a reason for hiding this comment

Uh oh!

kirrg001 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jan 8, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aryamohanan commented Sep 22, 2025 •

edited

Loading

aryamohanan commented Jan 7, 2026 •

edited

Loading