Skip to content

chore: upgrade nock to v14 and fix api-request unit tests#3153

Open
lahirumaramba wants to merge 1 commit into
v14from
lm-bump-nock
Open

chore: upgrade nock to v14 and fix api-request unit tests#3153
lahirumaramba wants to merge 1 commit into
v14from
lm-bump-nock

Conversation

@lahirumaramba
Copy link
Copy Markdown
Member

Upgrades nock to v14.0.15 and updates HttpClient unit tests to align with nock v14's spec-compliant response handling and socket interception refactors.

  • Bumps nock from ^13.0.0 to ^14.0.15 in package.json.
  • Updates mockRequestWithError to normalize mock error inputs into real Error objects, preventing nock v14 from silently dropping request error events.
  • In nock v13, a bug existed where nock incorrectly returned a response body for HEAD requests if respData was supplied to .reply(), forcing the old test to assert that resp.data was parsed. In nock v14, nock correctly conforms to the HTTP specification and ignores any reply body for HEAD requests, resulting in an empty raw response ("").
  • Removed the obsolete mock response body (respData) from the nock reply. The test now correctly asserts that resp.text === '' and that accessing resp.data throws a missing data error.
  • Refactors socket timeout tests to directly reply with an ETIMEDOUT error using replyWithError(err) instead of relying on mock socket delay events, which were not reliably triggering under nock v14.

@lahirumaramba lahirumaramba added the release:stage Stage a release candidate label May 19, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request upgrades the nock dependency to version 14 and updates the associated package-lock.json with new sub-dependencies. Corresponding unit tests in api-request.spec.ts were updated to handle HEAD requests without bodies and to simulate timeouts using replyWithError. Feedback was provided regarding the error normalization logic in mockRequestWithError, which currently lacks robust handling for string primitives or null/undefined inputs, potentially leading to runtime errors or incorrect property assignments.

Comment on lines +95 to +101
let errorInstance: Error;
if (err instanceof Error) {
errorInstance = err;
} else {
errorInstance = new Error(err.message || 'Test network error');
Object.assign(errorInstance, err);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The normalization logic in mockRequestWithError has a few potential issues:

  • If err is a string, err.message is undefined, causing the error message to default to 'Test network error'. Additionally, Object.assign on a string primitive will copy its indexed characters as properties (e.g., '0': 'f'), which is likely unintended.
  • If err is null or undefined, accessing err.message will throw a runtime error.

Consider handling strings explicitly and adding a null check before property access.

  let errorInstance: Error;
  if (err instanceof Error) {
    errorInstance = err;
  } else if (typeof err === 'string') {
    errorInstance = new Error(err);
  } else {
    errorInstance = new Error(err?.message || 'Test network error');
    if (err && typeof err === 'object') {
      Object.assign(errorInstance, err);
    }
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release:stage Stage a release candidate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant