Add api and model tests with jest#7
Merged
Merged
Conversation
Reviewer's GuideThis PR establishes a full Jest-based testing framework—adding configuration, scripts, dependencies, utilities, sample suites, and CI integration—while adjusting linting and formatting rules and updating documentation to reflect the new testing workflow. Class diagram for new test utilities moduleclassDiagram
class TestUtils {
+createMockConfiguration()
+createMockResponse()
+createMockFetch()
+createMockFetchSequence()
+setupFetchMock()
+restoreFetch()
+validateObjectStructure()
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive unit tests using Jest for API clients and model serialization. The tests ensure proper functionality of JSON serialization/deserialization, API calls, authentication, error handling, and data validation.
Key Changes:
- Added Jest test suite for Run, Result, and Project models covering type definitions, JSON conversion, and round-trip serialization
- Added comprehensive API tests for ResultApi and ProjectApi including CRUD operations, pagination, filtering, and authentication
- Added test utilities for mocking fetch responses and configurations
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/models/tests/Run.test.ts | Tests for Run model covering interface validation, JSON conversion (snake_case ↔ camelCase), null/undefined handling, and round-trip serialization |
| src/models/tests/Result.test.ts | Tests for Result model including enum values, JSON serialization, and round-trip conversion |
| src/models/tests/Project.test.ts | Tests for Project model focusing on field mapping, null handling, and data integrity |
| src/apis/tests/ResultApi.test.ts | Comprehensive API tests for ResultApi covering CRUD operations, filtering, pagination, authentication, and error handling |
| src/apis/tests/ProjectApi.test.ts | API tests for ProjectApi including project management operations, filter parameters, and authentication |
| src/tests/test-utils.ts | Utility functions for mocking fetch responses and creating test configurations |
| package.json | Added Jest-related dependencies and test scripts (watch mode and coverage) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider integrating setupFetchMock and restoreFetch into Jest’s global setup or beforeEach/afterEach hooks so that fetch is consistently mocked and cleaned up across all tests.
- The Response.clone helper currently returns the same object; to more closely mimic native behavior and avoid shared state you should have clone return a fresh copy of the response.
- Be sure your new API and model tests include scenarios for non-2xx responses and malformed payloads so that your mocks and utilities are validated against expected error paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider integrating setupFetchMock and restoreFetch into Jest’s global setup or beforeEach/afterEach hooks so that fetch is consistently mocked and cleaned up across all tests.
- The Response.clone helper currently returns the same object; to more closely mimic native behavior and avoid shared state you should have clone return a fresh copy of the response.
- Be sure your new API and model tests include scenarios for non-2xx responses and malformed payloads so that your mocks and utilities are validated against expected error paths.
## Individual Comments
### Comment 1
<location> `src/__tests__/test-utils.ts:20` </location>
<code_context>
+/**
+ * Create a mock fetch response
+ */
+export function createMockResponse<T>(
+ data: T,
+ status = 200,
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for the utility functions themselves.
Unit tests for these helpers should cover edge cases such as invalid input, error status codes, and header overrides.
</issue_to_address>
### Comment 2
<location> `src/__tests__/test-utils.ts:25-43` </location>
<code_context>
const response = {
ok: status >= 200 && status < 300,
status,
statusText: status === 200 ? 'OK' : 'Error',
headers: new Headers({
'Content-Type': 'application/json',
...headers,
}),
json: async () => data,
text: async () => JSON.stringify(data),
blob: async () => new Blob([JSON.stringify(data)]),
arrayBuffer: async () => new ArrayBuffer(0),
formData: async () => new FormData(),
clone: function () {
return this;
},
} as Response;
return response;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/inline-immediately-returned-variable))
```suggestion
return {
ok: status >= 200 && status < 300,
status,
statusText: status === 200 ? 'OK' : 'Error',
headers: new Headers({
'Content-Type': 'application/json',
...headers,
}),
json: async () => data,
text: async () => JSON.stringify(data),
blob: async () => new Blob([JSON.stringify(data)]),
arrayBuffer: async () => new ArrayBuffer(0),
formData: async () => new FormData(),
clone: function () {
return this;
},
} as Response;
```
<br/><details><summary>Explanation</summary>Something that we often see in people's code is assigning to a result variable
and then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
</details>
</issue_to_address>
### Comment 3
<location> `src/apis/__tests__/ProjectApi.test.ts:294` </location>
<code_context>
const headers = mockFetch.mock.calls[0][1].headers;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {headers} = mockFetch.mock.calls[0][1];
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 4
<location> `src/apis/__tests__/ProjectApi.test.ts:304` </location>
<code_context>
const headers = mockFetch.mock.calls[0][1].headers;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {headers} = mockFetch.mock.calls[0][1];
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 5
<location> `src/apis/__tests__/ResultApi.test.ts:381` </location>
<code_context>
const headers = mockFetch.mock.calls[0][1].headers;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {headers} = mockFetch.mock.calls[0][1];
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 6
<location> `src/apis/__tests__/ResultApi.test.ts:391` </location>
<code_context>
const headers = mockFetch.mock.calls[0][1].headers;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {headers} = mockFetch.mock.calls[0][1];
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
mshriver
force-pushed
the
ts-test-coverage
branch
from
dfbcec2 to
994ce82
Compare
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `eslint.config.mjs:207` </location>
<code_context>
// Special rules for auto-generated code
{
files: ['src/apis/**/*.ts', 'src/models/**/*.ts', 'src/runtime.ts'],
+ ignores: ['**/__tests__/**/*.ts', '**/*.test.ts', '**/*.spec.ts'],
rules: {
// Relax some strict rules for auto-generated OpenAPI code
</code_context>
<issue_to_address>
**issue:** Consider using 'excludedFiles' instead of 'ignores' for ESLint config.
'excludedFiles' is the standard field for ignoring files in ESLint overrides. Please verify that 'ignores' is supported in your ESLint configuration.
</issue_to_address>
### Comment 2
<location> `package.json:35` </location>
<code_context>
"format": "prettier --write \"src/**/*.ts\"",
- "format:check": "prettier --check \"src/**/*.ts\""
+ "format:check": "prettier --check \"src/**/*.ts\"",
+ "precommit": "yarn lint:fix && yarn test"
},
"repository": {
</code_context>
<issue_to_address>
**suggestion:** Consider using 'pre-commit' hook tooling for precommit scripts.
Most tools do not support a 'precommit' script in package.json. For running lint and tests before commits, use 'husky' or 'lint-staged' to integrate with git hooks.
Suggested implementation:
```
"format:check": "prettier --check \"src/**/*.ts\""
},
```
```
"devDependencies": {
"@eslint/js": "^9.16.0",
"@types/jest": "^29.5.14",
"@types/node": "^24.10.0",
"eslint": "^9.39.1",
"eslint-config-prettier": "^10.1.8",
"jest": "^30.2.0",
"prettier": "^3.4.0",
"husky": "^9.0.11",
"lint-staged": "^15.2.0"
```
```
"lint-staged": {
"src/**/*.ts": [
"eslint --fix",
"prettier --write",
"jest --bail --findRelatedTests"
]
},
"repository": {
"type": "git",
```
```
},
"scripts": {
"prepare": "husky install"
},
```
You must also run the following commands in your project root after updating package.json:
1. `yarn install` (to install husky and lint-staged)
2. `yarn husky install` (to initialize husky)
3. `yarn husky add .husky/pre-commit "npx lint-staged"` (to add the pre-commit hook)
This will ensure linting and tests run on staged files before each commit using proper git hook tooling.
</issue_to_address>
### Comment 3
<location> `eslint.config.mjs:226` </location>
<code_context>
'@typescript-eslint/prefer-optional-chain': 'off',
},
},
+ // Strict rules for test files
+ {
+ files: ['**/__tests__/**/*.ts', '**/*.test.ts', '**/*.spec.ts'],
</code_context>
<issue_to_address>
**issue (complexity):** Consider extending common ESLint presets and only overriding necessary rules to simplify the test file configuration.
You can collapse almost all of those one-off rules by extending the common presets and only overriding the handful of truly custom bits. For example, instead of enumerating every `no-unsafe-*` and promise rule by hand, you can do:
```js
// .eslintrc.js
const TEST_GLOB = [
'**/__tests__/**/*.ts',
'**/*.test.ts',
'**/*.spec.ts',
];
module.exports = {
overrides: [
// …other overrides…
{
files: TEST_GLOB,
// bring in the Jest and TS rules you already want
extends: [
'plugin:jest/recommended',
'plugin:@typescript-eslint/recommended-requiring-type-checking',
],
rules: {
// allow type inference in tests
'@typescript-eslint/explicit-function-return-type': 'off',
'@typescript-eslint/explicit-module-boundary-types': 'off',
// relax a couple test-specific bits
'@typescript-eslint/unbound-method': 'off',
'@typescript-eslint/no-non-null-assertion': 'warn',
'no-console': 'off',
// the presets already cover all of your no-unsafe-* and promise rules,
// so you only need to override the ones that really differ.
},
},
// …other overrides…
],
};
```
If you still need to force-enable a subset of rules that the presets don’t cover, you can group them via a small helper and spread:
```js
const NO_UNSAFE = [
'no-explicit-any',
'no-unsafe-assignment',
'no-unsafe-member-access',
'no-unsafe-call',
'no-unsafe-return',
'no-unsafe-argument',
].reduce((acc, r) => ({ ...acc, [`@typescript-eslint/${r}`]: 'error' }), {});
module.exports = {
overrides: [
{
files: TEST_GLOB,
extends: [ /* … */ ],
rules: {
...NO_UNSAFE,
// any other overrides…
},
},
],
};
```
Either approach keeps all existing checks while cutting ~40 lines of one-off definitions.
</issue_to_address>
### Comment 4
<location> `jest.config.js:2` </location>
<code_context>
+/** @type {import('jest').Config} */
+module.exports = {
+ preset: 'ts-jest',
+ testEnvironment: 'node',
</code_context>
<issue_to_address>
**issue (complexity):** Consider using the ts-jest preset and only overriding necessary options to simplify your Jest config.
Here’s one way to DRY this down without changing any behavior:
1. Pull in the standard ts-jest preset (it already gives you `transform`, `moduleFileExtensions`, default `testMatch`, default `coveragePathIgnorePatterns = ['/node_modules/']`, etc.).
2. Only override the bits you actually need (your `setupFilesAfterEnv`, `roots`, coverage thresholds, timeout, flags).
```js
// jest.config.js
const { defaults: tsjPreset } = require('ts-jest/presets')
/** @type {import('jest').Config} */
module.exports = {
// 1) spread in all of ts-jest's defaults
...tsjPreset,
// 2) override only what you really need:
roots: ['<rootDir>/src'],
setupFilesAfterEnv: ['<rootDir>/jest.setup.ts'],
// tests are still discovered by default ts-jest[testMatch]
// no need to repeat .test.ts/.spec.ts patterns
collectCoverageFrom: [
// include all .ts source
'src/**/*.ts',
// but ignore defs, indexes
'!src/**/*.d.ts',
'!src/**/index.ts',
],
// thresholds only
coverageThreshold: {
global: {
branches: 15,
functions: 15,
lines: 25,
statements: 25,
},
},
// only flags you changed from defaults
clearMocks: true,
errorOnDeprecated: true,
testTimeout: 10000,
verbose: true,
}
```
That shrinks you down from ~40 lines to about 15, removes all the duplicated ignore-patterns/reporters/extensions that ts-jest+Jest already provide, and keeps your custom thresholds, setup, and flags intact.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Integrate Jest-based testing for models and API clients with full configuration, coverage reporting, linting rules for tests, CI integration, and documentation updates
Enhancements:
Build:
CI:
Documentation:
Tests: