refactor: Use `GoogleAuth#fetch` and Fix Tests by d-goog · Pull Request #1766 · googleapis/gax-nodejs

d-goog · 2025-06-03T04:37:16Z

Description

This PR aims to:

resolve all silently failing tests*
fix tests that are misconfigured
establish improved patterns that will:
- increase test reliability
- improve maintainability
- improve test performance

Using GoogleAuth#fetch streamlines allows us to streamline the code and simplify the unit tests as we can use a native Response object.

*system-tests are not included and are still flaky.

Impact

In addition to the above, as a side-effect we no longer need to directly import node-fetch within this library. We can remove in a separate PR.

Testing

Lots of testing improvements.

Additional Information

Note: While a large PR there is minimal change to gax/src/
This PR reduces our usage of proxyquire, which won't be supported in an ESM future, along with building improved patterns around using mock data with real data types (without casting).
nock raises an AbortController error when ran due to our overwriting the native AbortController. Fortunately, the GoogleAuth#fetch/Response route is more performant, however we shouldn't modify/replace native objects (anti-pattern).

BEGIN_COMMIT_OVERRIDE
fix: passing gaxios fetch through auth
END_COMMIT_OVERRIDE

Fixes: googleapis/google-cloud-node-core#182 🦕

d-goog · 2025-06-03T04:42:44Z

gax/src/fallbackServiceStub.ts

      auth
-        .getRequestHeaders()
-        .then(authHeader => {
-          const fetchRequest: RequestInit = {
-            headers: gaxios.Gaxios.mergeHeaders(authHeader, headers) as {},
-            body: fetchParameters.body as string | Buffer | undefined,
-            method: fetchParameters.method,
-            signal: cancelSignal,
-          };
-          if (agentOption) {
-            fetchRequest.agent = agentOption;
-          }
-          if (
-            fetchParameters.method === 'GET' ||
-            fetchParameters.method === 'DELETE'
-          ) {
-            delete fetchRequest['body'];
-          }
-          return fetch(url, fetchRequest as {});
-        })
+        .fetch(url, fetchRequest)


Using GoogleAuth#fetch to streamline and improve mocking options.

gax/test/browser-test/tsconfig.json

d-goog · 2025-06-03T04:45:10Z

gax/test/unit/apiCallable.ts

    }
    const apiCall = createApiCall(func);
-    await apiCall(42, undefined, (err, resp) => {
+    void apiCall(42, undefined, (err, resp) => {


Used void to signal that this is a promise we intend to handle in floating async manner. The stubs by default are not compatible with traditional async/await patterns.

d-goog · 2025-06-03T04:46:34Z

gax/test/unit/apiCallable.ts

+        assert.ok(deadlineArg);
+        done();
+      })
+      .catch(done);


This .catch(done) will allow the test to fail faster than using .catch(console.error);

does this also need the void like the other tests?

re catch, great, love a faster fail

Good question - the ones with the .catch(done) don't require the void as the .catch handles the floating async lint warning

gax/test/unit/grpc-fallback.ts

d-goog · 2025-06-03T04:48:02Z

gax/test/unit/grpc-fallback.ts

+    setMockFallbackResponse(
+      gaxGrpc,
+      new Response(Buffer.from(JSON.stringify(response))),
+    );


This new utility makes it easier and simple to mock responses using the native Response object. Notice there are no fake mocks and no casting - just real data types which makes this easier to maintain.

d-goog · 2025-06-03T04:48:25Z

gax/test/unit/grpc-fallback.ts

-    const gaxGrpcMock = new GrpcClient();
+    setMockFallbackResponse(
+      gaxGrpc,
+      new Response(Buffer.from(JSON.stringify(jsonError)), {status: 400}),


...it even supports errors in a simple way

tsconfig.json

leahecole

it seems like there's a few things going on in this PR:

there's the work that was done to fix the fallback tests
there's also additional work - changing the browser tests
and the majority of these changes are updates to many of the unit tests and how we handle the async nature of them

The browser test changes are small, and given that they have some relevance to fetch + fallback from the linked issue, I would be okay grouping those in the PR, but I feel strongly that the other unit tests changes should be put in a separate PR (that I'm happy to review!)

.vscode/launch.json

gax/src/fallbackServiceStub.ts

gax/test/browser-test/test/test.endtoend.ts

gax/test/browser-test/tsconfig.json

tsconfig.json

gax/tsconfig.json

leahecole · 2025-06-03T15:17:15Z

gax/test/unit/apiCallable.ts

+        assert.ok(deadlineArg);
+        done();
+      })
+      .catch(done);


does this also need the void like the other tests?

re catch, great, love a faster fail

leahecole

Thanks for the some of the fixes to these comments! This PR still needs to be split up into multiple PRs that represent the discrete aspects of what is going on (fixing the fallback stuff, fixing the timing stuff, and potentially having the browser tests as a small third)- this will not only make it so #1737 is fixed faster (which means work on BigQuery modernization will be unblocked and has positive business impact! yay!) but having smaller PRs is safer in the unfortunate event that we did need to roll back a PR.

d-goog · 2025-06-04T21:39:17Z

Thanks for the some of the fixes to these comments! This PR still needs to be split up into multiple PRs that represent the discrete aspects of what is going on (fixing the fallback stuff, fixing the timing stuff, and potentially having the browser tests as a small third)- this will not only make it so googleapis/google-cloud-node-core#182 is fixed faster (which means work on BigQuery modernization will be unblocked and has positive business impact! yay!) but having smaller PRs is safer in the unfortunate event that we did need to roll back a PR.

As mentioned here, we will need the browser changes in the PR. To further expand:

Many tests were broken or at risk for breaking due to the async issues/swallowing of the unit tests
I've looked into alternatives, such as adopting nock, however those won't work without a large refactor as well (larger than this PR)
- For the nock case, adopting it caused tests to fail because of the way we mock AbortController (a global, btw), and fixing that would be larger
I've landed on using GoogleAuth#fetch since that would things up in the last amount of work (still a lot) and would allow us to use better practices going forward
- These changes remove node-fetch usage, which we were mocking, thus the browser tests needed updating as well

To expedite this I would be happy to provide a walkthrough of these changes.

leahecole · 2025-06-05T15:52:16Z

As mentioned here, we will need the browser changes in the PR.

I am willing to leave them in this PR, this is fine.

Many tests were broken or at risk for breaking due to the async issues/swallowing of the unit tests

Understood - however, linked issue googleapis/google-cloud-node-core#182 issue specifically calls out the grpc-fallback tests - I think that fixing all of the async issues and tests is valid, but needs to be split into two PRs:

One that specifically deals with the refactoring to use GoogleAuth#fetch (which I believe is the fix for the grpc-fallback stuff) - this may include some more tests than just the fallback ones - it'll be the ones that used proxyquire before and things that pertain to fallback in general (quickly going through the files, I think that there's also some in the regapic.ts unit test file that would logically be goruped with this as well.) This one can also have the browser tests, since it kind of has to do with fetch too.
One that handles the timing changes that are separate from this, but still need to be fixed - basically all of the unit tests where awaits were changed to voids and the .catchs were modified to fail faster - files including, but not limited to, apiCallable.ts and bundling.ts, locationService.ts, longrunning.ts, etc.

I've looked into alternatives, such as adopting nock, however those won't work without a large refactor as well (larger than this PR)

For the nock case, adopting it caused tests to fail because of the way we mock AbortController (a global, btw), and fixing that would be larger

I've landed on using GoogleAuth#fetch since that would things up in the last amount of work (still a lot) and would allow us to use better practices going forward

Understood, this makes sense. I think this is a reasonable alternative and a good choice given the circumstances. No issues with this choice.

These changes remove node-fetch usage, which we were mocking, thus the browser tests needed updating as well

To expedite this I would be happy to provide a walkthrough of these changes.

I appreciate the offer, but am happy to async review both PRs when this has been split up!

d-goog · 2025-06-05T16:19:38Z

As mentioned here, we will need the browser changes in the PR.

I am willing to leave them in this PR, this is fine.

Many tests were broken or at risk for breaking due to the async issues/swallowing of the unit tests

Understood - however, linked issue googleapis/google-cloud-node-core#182 issue specifically calls out the grpc-fallback tests - I think that fixing all of the async issues and tests is valid, but needs to be split into two PRs:

One that specifically deals with the refactoring to use GoogleAuth#fetch (which I believe is the fix for the grpc-fallback stuff) - this may include some more tests than just the fallback ones - it'll be the ones that used proxyquire before and things that pertain to fallback in general (quickly going through the files, I think that there's also some in the regapic.ts unit test file that would logically be goruped with this as well.) This one can also have the browser tests, since it kind of has to do with fetch too.

One that handles the timing changes that are separate from this, but still need to be fixed - basically all of the unit tests where awaits were changed to voids and the .catchs were modified to fail faster - files including, but not limited to, apiCallable.ts and bundling.ts, locationService.ts, longrunning.ts, etc.

I've looked into alternatives, such as adopting nock, however those won't work without a large refactor as well (larger than this PR)

For the nock case, adopting it caused tests to fail because of the way we mock AbortController (a global, btw), and fixing that would be larger

I've landed on using GoogleAuth#fetch since that would things up in the last amount of work (still a lot) and would allow us to use better practices going forward

Understood, this makes sense. I think this is a reasonable alternative and a good choice given the circumstances. No issues with this choice.

These changes remove node-fetch usage, which we were mocking, thus the browser tests needed updating as well

To expedite this I would be happy to provide a walkthrough of these changes.

I appreciate the offer, but am happy to async review both PRs when this has been split up!

Thanks for clarifying - I’ll split out the async/void changes from this PR.

Just noting here, that if the other async/void PR isn’t merged, this library would remain in an unstable state.

gax/src/fallbackServiceStub.ts

gax/src/grpc.ts

leahecole · 2025-06-05T17:35:48Z

gax/test/browser-test/test/test.grpc-fallback.ts

+          done();
+        });
+      })
+      .catch(done);


If the assertion fails in 278, it's not caught by the .catch - the test instead fails because it times out after 2000ms. Was the intent of the new .catch to avoid that? If not, and it's fulfilling another purpose, I suspect that we had this problem before and I'll open an issue, but if so, it might need a small adjustment

This catch is for the createStub call. We could further improve by fast-failing here - would you like me to add that?

given that we had this issue before, I will open up an issue to track and fast follow. The tests do fail, but we can make em "fail better". Opened a tracking issue https://github.com/googleapis/gax-nodejs/issues/1769

leahecole · 2025-06-05T17:44:07Z

gax/test/unit/grpc-fallback.ts

+    setMockFallbackResponse(
+      gaxGrpc,
+      new Response(JSON.stringify({}), {
+        status: 403,


why is the status 403 instead of OK like it was when we were attempting to proxyquire?

Good catch - it didn't make a difference for the tested functionality, but I can make it match.

leahecole · 2025-06-05T18:47:00Z

gax/test/unit/grpc-fallback.ts

          JSON.stringify(err.statusDetails),
          JSON.stringify(expectedError.details),
        );
+        done();


I'm not sure how quickly it was failing before, but right now even with the done in place, if the assertion error fails, it times out instead of failing fast

https://github.com/googleapis/gax-nodejs/issues/1769

leahecole · 2025-06-05T18:48:04Z

gax/test/unit/regapic.ts

+    it('should not send prettyPrint setting when json minification is not requested', done => {
+      const requestObject = {name: 'shelves/shelf-name'};
+      const responseObject = {
+        name: 'shelf-name',
+        theme: 'shelf-theme',
+        type: 100, // unknown enum value


what are these json minification tests? did they not exist before?

That's strange - I merged and repaired from main and I fixed them here. Is this something you want removed or spun-off?

ugh hate when git syncs don't go as planned!
yeah if it doesn't have to do with this PR, let's spin it off into another in the name of small PRs please!

leahecole · 2025-06-06T12:54:40Z

gax/test/unit/regapic.ts

+    });
+  });
+
+  describe('should support json minification', () => {


added #1770 to track this quick re-add!

feat: Use GoogleAuth#fetch and Fix Tests

7074e23

d-goog requested a review from a team June 3, 2025 04:37

d-goog requested a review from a team as a code owner June 3, 2025 04:37

product-auto-label bot added the size: xl Pull request size is extra large. label Jun 3, 2025

d-goog commented Jun 3, 2025

View reviewed changes

gax/test/browser-test/tsconfig.json Show resolved Hide resolved

d-goog commented Jun 3, 2025

View reviewed changes

gax/test/unit/grpc-fallback.ts Show resolved Hide resolved

d-goog commented Jun 3, 2025

View reviewed changes

tsconfig.json Show resolved Hide resolved

leahecole suggested changes Jun 3, 2025

View reviewed changes

d-goog added 6 commits June 3, 2025 09:00

chore: remove launch.json

4930daa

style: lint

25d892c

chore: remove unnecessary component

7af4aa8

refactor: remove unneeded globals

6fe944f

remove unused import

a58b2b6

refactor: base tsconfig.json

4a18913

d-goog changed the title ~~feat: Use GoogleAuth#fetch and Fix Tests~~ refactor: Use GoogleAuth#fetch and Fix Tests Jun 3, 2025

d-goog added 5 commits June 3, 2025 13:28

feat: open baseDirectory option (internal-only for now)

c9ef48a

refactor: streamline options

6c740cd

fix: use correct reference

aca1657

test: more test fixes

2ca4905

test: update browser tests

ebbd8c4

d-goog requested a review from leahecole June 4, 2025 16:55

leahecole suggested changes Jun 4, 2025

View reviewed changes

d-goog requested a review from leahecole June 4, 2025 22:11

chore: spin-off async/void tests

8c7ab66

d-goog mentioned this pull request Jun 5, 2025

test: Fix asynctest issues #1767

Merged

leahecole suggested changes Jun 5, 2025

View reviewed changes

refactor: pr suggestions

50512f9

This was referenced Nov 3, 2025

cleanup: finish removing node-fetch types from fallbackServiceStub googleapis/google-cloud-node-core#178

Open

cleanup: fail faster in tests using .done googleapis/google-cloud-node-core#177

Closed

refactor: remove minification test

e47e91f

d-goog requested a review from leahecole June 6, 2025 02:11

leahecole mentioned this pull request Nov 3, 2025

add json minification tests for regapic googleapis/google-cloud-node-core#176

Closed

remove tests as talked about on chat, see #1770 for context

1f5fdbe

leahecole reviewed Jun 6, 2025

View reviewed changes

leahecole self-assigned this Jun 6, 2025

leahecole approved these changes Jun 6, 2025

View reviewed changes

leahecole mentioned this pull request Jun 6, 2025

build: re add minification tests #1771

Merged

sofisl approved these changes Jun 6, 2025

View reviewed changes

leahecole merged commit e7c686a into main Jun 6, 2025
28 checks passed

leahecole deleted the fix-gax-tests branch June 6, 2025 19:06

leahecole mentioned this pull request Jun 6, 2025

fix!: allow 204 responses in REST fallback mode #1736

Merged

Conversation

d-goog commented Jun 3, 2025 • edited by sofisl Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Impact

Testing

Additional Information

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

d-goog Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

d-goog Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

leahecole left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leahecole left a comment

Choose a reason for hiding this comment

Uh oh!

d-goog commented Jun 4, 2025

Uh oh!

leahecole commented Jun 5, 2025

Uh oh!

d-goog commented Jun 5, 2025

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

d-goog commented Jun 3, 2025 •

edited by sofisl

Loading

d-goog Jun 3, 2025 •

edited

Loading

d-goog Jun 3, 2025 •

edited

Loading