Skip to content
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

Jump to bottom

test(next-drupal): reorganize Jest tests #675

Merged
merged 4 commits into from
Feb 21, 2024
Merged

test(next-drupal): reorganize Jest tests #675

merged 4 commits into from
Feb 21, 2024

Conversation

JohnAlbin
Copy link
Collaborator

@JohnAlbin JohnAlbin commented Feb 5, 2024 •
edited
Loading

This pull request is for:

  • packages/next-drupal

GitHub Issue: #608

Describe your changes

  1. Breaks Jest tests into multiple files to make them easier to maintain.

  2. It also moves get-* functions into src/deprecated and splits types.ts into multiple files to make it easier to know what code has missing tests (and is not deprecated). I've removed the pre-1.6 code from code coverage for this reason; there's little reason to add missing tests to code that is going to be phased out.

FYI, moving the src/ code around doesn't affect the named entry points for next-drupal; those remain the same.

@vercel Vercel
Copy link

vercel bot commented Feb 5, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
next-drupal ⬜️ Ignored (Inspect) Visit Preview Feb 21, 2024 2:17pm

@JohnAlbin JohnAlbin force-pushed the 608-jest-organization branch 4 times, most recently from 146aff1 to c37549e Compare February 8, 2024 12:03
@JohnAlbin JohnAlbin force-pushed the 608-jest-organization branch 2 times, most recently from 709cd80 to f7b5df3 Compare February 20, 2024 14:22
JohnAlbin
JohnAlbin commented Feb 20, 2024
View reviewed changes
packages/next-drupal/src/client.ts
@@ -1484,7 +1475,7 @@ export class DrupalClient {
throw error
}

private async handleJsonApiErrors(response: Response) {
private async throwIfJsonApiErrors(response: Response) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I renamed this private method to better describe what happens in the method.

packages/next-drupal/src/client.ts
JsonApiWithCacheOptions,
JsonApiWithLocaleOptions,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I renamed some type definitions.

packages/next-drupal/src/client.ts
@@ -264,7 +264,7 @@ export class DrupalClient {
if (token) {
init["headers"]["Authorization"] = `Bearer ${token.access_token}`
}
} else if (isAccessTokenAuth(this._auth)) {
} /* c8 ignore next 4 */ else if (isAccessTokenAuth(this._auth)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I got the code coverage to about 99% coverage. These little c8 ignore next comments have been added as a little cheat to make the coverage reach 100%. We can come back and remove them as the tests are improved.

packages/next-drupal/src/client.ts
@@ -312,7 +312,7 @@ export class DrupalClient {
async createResource<T extends JsonApiResource>(
type: string,
body: JsonApiCreateResourceBody,
options?: JsonApiWithLocaleOptions & JsonApiWithAuthOptions
options?: JsonApiOptions
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The JsonApiWithLocaleOptions & JsonApiWithAuthOptions were always used together so it didn't make sense to have them be two separate types anymore. (They did need to be separate for the pre-1.6 API.)

packages/next-drupal/src/client.ts
if (!response?.ok) {
await this.handleJsonApiErrors(response)
}
await this.throwIfJsonApiErrors(response)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The handleJsonApiErrors method was already checking if (!response?.ok) inside it, so the wrapping if is redundant. The method has been renamed to make it clear how it is handling the errors.

packages/next-drupal/src/jsonapi-errors.ts
// https://jsonapi.org/format/#document-links
export interface JsonApiLinks {
[key: string]: string | Record<string, string>
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved this out of the super long types.ts because these interfaces describe the class defined in this file.

packages/next-drupal/src/types/client.ts
body: Record<string, unknown>,
options?: Record<string, unknown>
): unknown
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

197 lines long is better than 478 lines long.

packages/next-drupal/jest.config.cjs
},
testLocationInResults: true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The testLocationInResults flag shows the directory and filename of all the test.ts files which makes it way easier to organize the tests and quickly see where a failing test is.

packages/next-drupal/jest.config.cjs
"./src/deprecated/*",
"./src/deprecated.ts",
"./src/navigation.ts",
"./src/types/*",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's no reason to track code coverage of the deprecated API. And TypeScript removes import type lines from generated JS, so the JS code coverage tool thinks the type definition files are 0% used in tests.

packages/next-drupal/jest.config.cjs
statements: 100,
branches: 100,
functions: 100,
lines: 100,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yay! 100% code coverage.

@JohnAlbin JohnAlbin marked this pull request as ready for review February 20, 2024 16:35
@JohnAlbin JohnAlbin force-pushed the 608-jest-organization branch from f7b5df3 to 807a1d6 Compare February 21, 2024 05:18
@JohnAlbin
refactor(next-drupal)!: move non-DrupalClient code to src/deprecated
2c4028c 
BREAKING CHANGE:
The JsonApiWithAuthOptions type has been renamed to JsonApiWithAuthOption. The
JsonApiWithLocaleOptions type is now considered deprecated for DrupalClient
usage and can be replaced with the JsonApiOptions type.
@JohnAlbin JohnAlbin force-pushed the 608-jest-organization branch from 807a1d6 to 242aa59 Compare February 21, 2024 09:26
@JohnAlbin JohnAlbin force-pushed the 608-jest-organization branch from 242aa59 to 50642c5 Compare February 21, 2024 09:32
robdecker
robdecker approved these changes Feb 21, 2024
View reviewed changes
Copy link
Member

@robdecker robdecker left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@JohnAlbin JohnAlbin merged commit cc092c8 into main Feb 21, 2024
12 checks passed
@JohnAlbin JohnAlbin deleted the 608-jest-organization branch February 21, 2024 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robdecker robdecker robdecker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JohnAlbin @robdecker