feat: migrate paginator to jest - #9218
Conversation
…hub.com/googleapis/google-cloud-node into pearigee-migrate-core-paginator-to-jest
…hub.com/googleapis/google-cloud-node into pearigee-migrate-core-paginator-to-jest
This reverts commit 5bf3fa9.
This reverts commit c2bd98b.
…into pearigee-migrate-core-paginator-to-jest-3 # Conflicts: # core/paginator/.mocharc.js # core/paginator/jest.config.js # core/paginator/package.json # core/paginator/test/index.ts # handwritten/google-cloud-dns/.mocharc.js
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the test suite from Mocha, Sinon, and Assert to Jest, updating the configuration files, dependencies, and test files accordingly. Feedback on these changes includes correcting an accidental downgrade of JSDoc dependencies in package.json, refactoring duplicate mock class definitions and misplaced ES imports in core/paginator/test/index.ts, and utilizing more idiomatic Jest matchers and properties like toHaveBeenLastCalledWith and mock.lastCall in core/paginator/test/resource-stream.ts.
| class FakeResourceStream extends Transform { | ||
| calledWith: IArguments; | ||
| constructor() { | ||
| calledWith: any[]; | ||
| constructor(...args: any[]) { | ||
| super({objectMode: true}); | ||
| /* eslint-disable-next-line prefer-rest-params */ | ||
| this.calledWith = arguments; | ||
| this.calledWith = args; | ||
| } | ||
| } | ||
|
|
||
| const p = proxyquire('../src', { | ||
| './resource-stream': {ResourceStream: FakeResourceStream}, | ||
| }) as typeof P; | ||
| // Mock the resource-stream module so runAsStream_ instantiates the fake class | ||
| jest.mock('../src/resource-stream', () => { | ||
| const {Transform} = require('stream'); | ||
| return { | ||
| ResourceStream: class FakeResourceStream extends Transform { | ||
| calledWith: any[]; | ||
| constructor(...args: any[]) { | ||
| super({objectMode: true}); | ||
| this.calledWith = args; | ||
| } | ||
| }, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
There is duplicate code defining FakeResourceStream twice: once as a local class and once inside the jest.mock factory. In Jest, you can reference out-of-scope variables inside jest.mock if they are prefixed with mock. Renaming the class to mockFakeResourceStream allows you to reuse it directly in the mock factory, eliminating the duplicate class definition.
class mockFakeResourceStream extends Transform {
calledWith: any[];
constructor(...args: any[]) {
super({objectMode: true});
this.calledWith = args;
}
}
// Mock the resource-stream module so runAsStream_ instantiates the fake class
jest.mock('../src/resource-stream', () => {
return {
ResourceStream: mockFakeResourceStream,
};
});There was a problem hiding this comment.
This has been addressed.
|
|
||
| const sandbox = sinon.createSandbox(); | ||
| // Retrieve the mocked ResourceStream class to check instances | ||
| import {ResourceStream} from '../src/resource-stream'; |
There was a problem hiding this comment.
ES imports are hoisted to the top of the file by the compiler/transpiler. Placing them in the middle of the file can be misleading. Since jest.mock is also hoisted by Jest, you can safely move this import to the top of the file with the other imports to adhere to standard style guidelines.
|
|
||
| stream._read(); | ||
| const callback = requestSpy.lastCall.args[1]; | ||
| const callback = requestSpy.mock.calls[requestSpy.mock.calls.length - 1][1]; |
There was a problem hiding this comment.
In Jest 28+, you can use mock.lastCall to access the arguments of the last call directly, which is much cleaner and more readable than mock.calls[mock.calls.length - 1].
| const callback = requestSpy.mock.calls[requestSpy.mock.calls.length - 1][1]; | |
| const callback = requestSpy.mock.lastCall![1]; |
There was a problem hiding this comment.
Did a search and replace for this one.
danieljbruce
changed the title
…ithub.com/googleapis/google-cloud-node into pearigee-migrate-core-paginator-to-jest-3
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Reopening in https://github.com/googleapis/google-cloud-node/pull/9224/changes to pass the CLA check |
## Description Migrates the `@google-cloud/paginator` package test suite and configuration from Mocha, Sinon, and c8 to Jest and `ts-jest`. Key changes include: * **Dependency & Infrastructure Updates**: * Removed legacy test dependencies (`mocha`, `@types/mocha`, `sinon`, `@types/sinon`, `c8`, `proxyquire`, `@types/proxyquire`). * Added `jest`, `ts-jest`, and `@types/jest`. * Added `jest.config.js` configured with `ts-jest`. * Removed `.nycrc` and updated `.gitignore` for `coverage/`. * Updated `package.json` test script to use `jest --coverage`. * **Test Suite Migration**: * Migrated `test/index.ts` and `test/resource-stream.ts` to native Jest assertions (`expect(...).toBe()`, `expect(...).toEqual()`, etc.). * Replaced `proxyquire` and Sinon sandboxes/stubs/spies/timers with `jest.mock()`, `jest.spyOn()`, `jest.fn()`, and `jest.useFakeTimers()`. * Modernized asynchronous test cases to use `async`/`await` and wrapped callback-based `done()` handlers in `try/catch` blocks to prevent hanging on test failures. commits were migrated from #9218 to pass the cla check. ## Impact * **Maintainability & Consistency**: Aligns `@google-cloud/paginator` with repository-wide efforts to standardize test infrastructure on Jest. * **Developer Experience**: Faster test execution, native TypeScript handling via `ts-jest`, and integrated code coverage reporting. * **No Runtime Impact**: Purely internal test and development dependency changes; no public APIs or runtime behaviors were modified.
2 participants
experiment with the merge