Skip to content

test: replace proxyquire with sinon in gax, common, error-reporting, translate #9638

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.

Sign up for GitHub

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

Closed
danieljbruce wants to merge 1 commit into from
Closed

test: replace proxyquire with sinon in gax, common, error-reporting, translate #9638

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/packages/gax/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@
"scripts": {
"docs": "jsdoc -c .jsdoc.js",
"pretest": "pnpm run compile",
"test": "node ../../../bin/run-test.cjs --proxyquire-shim build/test/unit",
"test": "node ../../../bin/run-test.cjs build/test/unit",
"lint": "gts check src samples test",
"clean": "gts clean",
"compile": "node ./scripts/copy-protos.js && tsc -p . && cp src/*.json build/src && cp -r test/fixtures build/test && cp -r protos build/",
Expand Down
35 changes: 17 additions & 18 deletions core/packages/gax/test/unit/grpc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,8 @@

import assert from 'assert';
import * as path from 'path';
import proxyquire from 'proxyquire';
import * as sinon from 'sinon';
import {mkdirSync, writeFileSync} from 'fs';
import fs, {mkdirSync, writeFileSync} from 'fs';
import * as fsp from 'fs/promises';
import {afterEach, describe, it, beforeEach} from 'mocha';

Expand Down Expand Up @@ -613,28 +612,28 @@ describe('grpc', () => {
const originPath = path.join('test', 'path', 'location');
const includePath = path.join('example', 'import.proto');

afterEach(() => {
sinon.restore();
});

it('should throw an error if a file is not found', () => {
const findIncludePath = proxyquire('../../src/grpc', {
fs: {
existsSync: () => {
return false;
},
},
}).GoogleProtoFilesRoot._findIncludePath;
assert.throws(findIncludePath.bind(null, originPath, includePath));
sinon.stub(fs, 'existsSync').returns(false);
assert.throws(
GoogleProtoFilesRoot._findIncludePath.bind(
null,
originPath,
includePath,
),
);
});

it('should return the correct resolved import path', () => {
const correctPath = path.join('test', 'example', 'import.proto');
const findIncludePath = proxyquire('../../src/grpc', {
fs: {
existsSync(path: string) {
return path === correctPath;
},
},
}).GoogleProtoFilesRoot._findIncludePath;
sinon
.stub(fs, 'existsSync')
.callsFake((p: fs.PathLike) => p === correctPath);
assert.strictEqual(
findIncludePath(originPath, includePath),
GoogleProtoFilesRoot._findIncludePath(originPath, includePath),
correctPath,
);
});
Expand Down
2 changes: 1 addition & 1 deletion core/packages/nodejs-googleapis-common/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"prebenchmark": "npm run compile",
"benchmark": "node build/benchmark/bench.js",
"compile": "tsc -p .",
"test": "node ../../../bin/run-test.cjs --proxyquire-shim build/test",
"test": "node ../../../bin/run-test.cjs build/test",
"system-test": "node ../../../bin/run-test.cjs build/system-test --timeout 600000",
"presystem-test": "npm run compile",
"fix": "gts fix",
Expand Down
25 changes: 12 additions & 13 deletions core/packages/nodejs-googleapis-common/test/test.authplus.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,23 +13,17 @@
// limitations under the License.

import * as assert from 'assert';
import {describe, it} from 'mocha';
import * as proxyquire from 'proxyquire';
import * as AuthPlus from '../src/authplus';
import {afterEach, describe, it} from 'mocha';
import * as sinon from 'sinon';
import {AuthPlus} from '../src/authplus';

describe(__filename, () => {
const AuthyPlus: typeof AuthPlus = proxyquire('../src/authplus', {
'google-auth-library': {
GoogleAuth: class {
async getClient() {
return {};
}
},
},
afterEach(() => {
sinon.restore();
});

it('should expose auth classes', () => {
const auth = new AuthyPlus.AuthPlus();
const auth = new AuthPlus();
assert.ok(auth.JWT);
assert.ok(auth.Compute);
assert.ok(auth.OAuth2);
Expand All @@ -40,8 +34,13 @@ describe(__filename, () => {
});

it('should get a new client', async () => {
const auth = new AuthyPlus.AuthPlus();
const auth = new AuthPlus();
const getClientStub = sinon
.stub(auth.GoogleAuth.prototype, 'getClient')
// eslint-disable-next-line @typescript-eslint/no-explicit-any
.resolves({} as any);
Comment on lines +38 to +41

@gemini-code-assist gemini-code-assist Bot Oct 15, 2026

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

Instead of stubbing the prototype of GoogleAuth (which mutates the class prototype globally for all instances), you can stub the getClient method directly on the auth instance. This is safer, avoids prototype pollution, and is more idiomatic.

Suggested change
const getClientStub = sinon
.stub(auth.GoogleAuth.prototype, 'getClient')
// eslint-disable-next-line @typescript-eslint/no-explicit-any
.resolves({} as any);
const getClientStub = sinon
.stub(auth, 'getClient')
// eslint-disable-next-line @typescript-eslint/no-explicit-any
.resolves({} as any);

const client = await auth.getClient();
assert.deepStrictEqual(client, {});
assert.strictEqual(getClientStub.calledOnce, true);
});
});
21 changes: 11 additions & 10 deletions core/packages/nodejs-googleapis-common/test/test.http2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,13 @@

import * as assert from 'assert';
import * as coreHttp2 from 'http2';
import {describe, it, before, beforeEach} from 'mocha';
import {describe, it, before, beforeEach, after} from 'mocha';
import {gzipSync} from 'zlib';
import * as proxyquire from 'proxyquire';
import * as sinon from 'sinon';
import {Duplex, Readable} from 'stream';
import {EventEmitter} from 'events';
import {GaxiosResponse} from 'gaxios';
import * as http2Types from '../src/http2';
import * as http2 from '../src/http2';

import {GoogleAuth} from '../src';

Expand Down Expand Up @@ -59,13 +58,15 @@ describe('http2', () => {
return (connectStub || (() => {}))(host);
}
let connectStub: Function;
let http2: typeof http2Types;
let connectSinonStub: sinon.SinonStub;
before(() => {
http2 = proxyquire('../src/http2', {
http2: {
connect: fakeConnect,
},
});
connectSinonStub = sinon
.stub(coreHttp2, 'connect')
// eslint-disable-next-line @typescript-eslint/no-explicit-any
.callsFake(fakeConnect as any);
});
after(() => {
connectSinonStub.restore();
});

let requestStream: Duplex;
Expand Down Expand Up @@ -153,7 +154,7 @@ describe('http2', () => {
requestStream.push(Buffer.from('{}'));
requestStream.push(null);
await resPromise;
await new Promise(r => setTimeout(r, 500));
await new Promise(resolve => setTimeout(resolve, 500));
assert.strictEqual(Object.keys(http2.sessions).length, 0);
});

Expand Down
2 changes: 1 addition & 1 deletion handwritten/error-reporting/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"lint": "gts check",
"presystem-test": "pnpm run compile",
"system-test": "MOCHA_THROW_DEPRECATION=false node ../../bin/run-test.cjs build/system-test",
"test": "node ../../bin/run-test.cjs --proxyquire-shim build/test/unit",
"test": "node ../../bin/run-test.cjs build/test/unit",
"clean": "gts clean",
"compile": "tsc -p .",
"fix": "gts fix",
Expand Down
48 changes: 23 additions & 25 deletions handwritten/error-reporting/test/unit/google-apis/auth-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,15 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import * as proxyquire from 'proxyquire';
import {util} from '@google-cloud/common';
import {describe, beforeEach, afterEach, it} from 'mocha';

import {
Configuration,
ConfigurationOptions,
Logger,
} from '../../../src/configuration';
import {RequestHandler} from '../../../src/google-apis/auth-client';
import {deepStrictEqual} from '../../util';

function verifyReportedMessage(
Expand All @@ -28,28 +29,6 @@ function verifyReportedMessage(
expectedLogs: {error?: string; info?: string; warn?: string},
done: () => void,
) {
class ServiceStub {
authClient: {};
request: {};
constructor() {
this.authClient = {
async getAccessToken() {
if (errToReturn) {
throw errToReturn;
}
return 'some-token';
},
};
this.request = () => {};
}
}

const RequestHandler = proxyquire('../../../src/google-apis/auth-client', {
'@google-cloud/common': {
Service: ServiceStub,
},
}).RequestHandler;

const logs: {error?: string; info?: string; warn?: string} = {};
const logger = {
error(text: string) {
Expand All @@ -72,8 +51,27 @@ function verifyReportedMessage(
},
} as {} as Logger;
const config2 = new Configuration(config1, logger);
// tslint:disable-next-line:no-unused-expression
new RequestHandler(config2, logger);
const origMakeAuthFactory = util.makeAuthenticatedRequestFactory;
const origRequest = RequestHandler.prototype.request;
util.makeAuthenticatedRequestFactory = (() => ({
authClient: {
async getAccessToken() {
if (errToReturn) {
throw errToReturn;
}
return 'some-token';
},
},
getCredentials: {},
})) as unknown as typeof util.makeAuthenticatedRequestFactory;
RequestHandler.prototype.request = (() => {}) as typeof origRequest;
try {
// tslint:disable-next-line:no-unused-expression
new RequestHandler(config2, logger);
} finally {
util.makeAuthenticatedRequestFactory = origMakeAuthFactory;
RequestHandler.prototype.request = origRequest;
}
setImmediate(() => {
deepStrictEqual(logs, expectedLogs);
done();
Expand Down
2 changes: 1 addition & 1 deletion packages/google-cloud-translate/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
"postpack": "minifyProtoJson",
"samples-test": "cd samples/ && npm link ../ && npm i && npm test",
"system-test": "node ../../bin/run-test.cjs --config ../../.mocharc.cjs --no-parallel build/system-test",
"test": "node ../../bin/run-test.cjs --proxyquire-shim --config ../../.mocharc.cjs build/test"
"test": "node ../../bin/run-test.cjs --config ../../.mocharc.cjs build/test"
},
"dependencies": {
"@google-cloud/common": "^8.0.1",
Expand Down
43 changes: 30 additions & 13 deletions packages/google-cloud-translate/test/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import * as pfy from '@google-cloud/promisify';
import * as assert from 'assert';
import {after, afterEach, before, beforeEach, describe, it} from 'mocha';
import * as extend from 'extend';
import * as proxyquire from 'proxyquire';
import * as Module from 'module';
import * as r from 'request';

import * as orig from '../src';
Expand All @@ -44,7 +44,7 @@ const fakeUtil = extend({}, util, {
makeRequest(
opts: DecorateRequestOptions,
cfg: MakeRequestConfig,
cb: BodyResponseCallback
cb: BodyResponseCallback,
) {
if (makeRequestOverride) {
return makeRequestOverride(opts, cfg, cb);
Expand Down Expand Up @@ -74,13 +74,30 @@ describe('Translate v2', () => {
let translate: any;

before(() => {
Translate = proxyquire('../src/v2', {
const stubs: Record<string, unknown> = {
'@google-cloud/common': {
util: fakeUtil,
Service: FakeService,
},
'@google-cloud/promisify': fakePromisify,
}).Translate;
};
const mod = Module as unknown as {prototype: {require: NodeRequire}};
const origRequire = mod.prototype.require;
const v2Path = require.resolve('../src/v2');
const cached = require.cache[v2Path];
delete require.cache[v2Path];
mod.prototype.require = Object.assign(function (
this: NodeModule,
id: string,
) {
return id in stubs ? stubs[id] : origRequire.call(this, id);
}, origRequire);
try {
Translate = require('../src/v2').Translate;
} finally {
mod.prototype.require = origRequire;
if (cached) require.cache[v2Path] = cached;
}
Comment on lines +77 to +100

@gemini-code-assist gemini-code-assist Bot Oct 15, 2026

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.

high

Overriding Module.prototype.require is a fragile hack that relies on Node.js internal APIs and can easily break across different Node versions or alternative runtimes (like Bun or Deno). Since @google-cloud/common and @google-cloud/promisify are already loaded and cached in require.cache, you can simply mutate their exported properties on the cached module objects before requiring ../src/v2, and restore them in the finally block. This is much cleaner, safer, and standard.

    const common = require('@google-cloud/common');
    const origUtil = common.util;
    const origService = common.Service;
    const origPromisifyAll = pfy.promisifyAll;

    common.util = fakeUtil;
    common.Service = FakeService;
    pfy.promisifyAll = fakePromisify;

    const v2Path = require.resolve('../src/v2');
    const cached = require.cache[v2Path];
    delete require.cache[v2Path];
    try {
      Translate = require('../src/v2').Translate;
    } finally {
      common.util = origUtil;
      common.Service = origService;
      pfy.promisifyAll = origPromisifyAll;
      if (cached) require.cache[v2Path] = cached;
    }

});

beforeEach(() => {
Expand Down Expand Up @@ -266,7 +283,7 @@ describe('Translate v2', () => {
assert.strictEqual(apiResponse_, apiResponse);
assert.deepStrictEqual(apiResponse_, originalApiResponse);
done();
}
},
);
});
});
Expand Down Expand Up @@ -314,7 +331,7 @@ describe('Translate v2', () => {
assert.strictEqual(languages, null);
assert.strictEqual(apiResponse_, apiResponse);
done();
}
},
);
});
});
Expand Down Expand Up @@ -359,7 +376,7 @@ describe('Translate v2', () => {
assert.deepStrictEqual(languages, expectedResults);
assert.strictEqual(apiResponse_, apiResponse);
done();
}
},
);
});
});
Expand Down Expand Up @@ -407,7 +424,7 @@ describe('Translate v2', () => {
from: SOURCE_LANG_CODE,
to: TARGET_LANG_CODE,
},
assert.ifError
assert.ifError,
);
});
});
Expand Down Expand Up @@ -496,7 +513,7 @@ describe('Translate v2', () => {
assert.strictEqual(translations, null);
assert.strictEqual(resp, apiResponse);
done();
}
},
);
});
});
Expand Down Expand Up @@ -531,7 +548,7 @@ describe('Translate v2', () => {
assert.deepStrictEqual(translations, expectedResults);
assert.strictEqual(resp, apiResponse);
done();
}
},
);
});

Expand All @@ -552,7 +569,7 @@ describe('Translate v2', () => {
assert.ifError(err);
assert.deepStrictEqual(translations, [expectedResults]);
done();
}
},
);
});
});
Expand Down Expand Up @@ -584,7 +601,7 @@ describe('Translate v2', () => {

FakeService.prototype.request = (
reqOpts: r.Options,
callback: Function
callback: Function,
) => {
assert.strictEqual(reqOpts, fakeOptions);
callback();
Expand Down Expand Up @@ -640,7 +657,7 @@ describe('Translate v2', () => {
makeRequestOverride = (
reqOpts: r.Options,
options: {},
callback: Function
callback: Function,
) => {
assert.deepStrictEqual(reqOpts, expectedReqOpts);
assert.strictEqual(options, translate.options);
Expand Down
Loading