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.

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

chore: Add jest unit testing #67

Merged
merged 40 commits into from
Jan 27, 2024
Merged

chore: Add jest unit testing #67

merged 40 commits into from
Jan 27, 2024

Conversation

jfoo1984
Copy link
Collaborator

@jfoo1984 jfoo1984 commented Jan 13, 2024

Pull Request

JIRA Ticket

CZID-9015

Description

Add unit tests based on graphQL mesh examples.

  • Set up jest and related config
  • Tests added:
    • tests/UnifiedSchema.test.ts
    • tests/ZipLinkQuery.test.ts

Notes

I originally mocked data with the mock plugin, but it was pointed out that this doesn't test our resolvers. I've switched approaches to mock data retrieval methods called by the resolvers. But I created a branch with the mock store approach, in case we want to use that in the future: https://github.com/chanzuckerberg/czid-graphql-federation-server/tree/testing-mock-store-example

Tests

This PR adds automated tests

@jfoo1984 jfoo1984 marked this pull request as ready for review January 24, 2024 06:16
@jfoo1984 jfoo1984 requested review from saq7 and rainandbare January 24, 2024 06:16
@@ -33,7 +33,7 @@
}
}
},
"report_table_data": {
"amr_hit": {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Align to schema returned by resolver. This was preventing the tests from running, because the schema could not be generated

@@ -104,7 +104,7 @@
"type": "object",
"properties": {
"tax_id": {
"type": "integer"
"type": "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.

Align to schema returned by resolver. This was preventing the tests from running, because the schema could not be generated

@@ -12,4 +12,4 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: chanzuckerberg/github-actions/.github/actions/conventional-commits@v1.3.2
- uses: chanzuckerberg/github-actions/.github/actions/conventional-commits@main
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since #61 was closed in favor of this PR, I am pulling this change in that was there.

@jfoo1984 jfoo1984 requested a review from jgadling January 24, 2024 19:45
@jfoo1984 jfoo1984 force-pushed the CZID-9015-add-snapshot-tests branch from 57824be to 9c2dd99 Compare January 25, 2024 05:30
Comment on lines +2 to +3
release:
types: [published]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is in preparation for changing the release cadence. stop pushing every PR to staging

@rainandbare
Copy link
Contributor

I like it Jerry! Great work!

I couldn't quite make the Ziplink test fail on its own unless I deleted it from the yaml so maybe we would have to write more specific tests? But even having the unified schema test is a logical and important safe guard.

Anything we can do about these messages?
Screenshot 2024-01-25 at 10 51 09 AM

@jfoo1984
Copy link
Collaborator Author

I was able to remove the mock plugin, and instead, use jest mocking of our httpUtil function to actually test the resolvers instead of bypassing them. This is probably a better test pattern we should use, given how much code we have in resolvers

The warnings are a type issue in the mock plugin, which I've raised with The Guild team. With the removal of the mock plugin though, the warnings should no longer appear

@jfoo1984
Copy link
Collaborator Author

I verified locally that changing code in the ZipLink resolver (to return a different response) does break the test.

Copy link
Contributor

@rainandbare rainandbare left a comment

Choose a reason for hiding this comment

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

I think this looks great Jerry! Are you worried that it is too much work per test?

@jfoo1984
Copy link
Collaborator Author

I think this looks great Jerry! Are you worried that it is too much work per test?

Thanks for reviewing @rainandbare! Are you referring to added engineering work to write these tests? My thought is that the amount of work to add these tests is inline with what I would expect for writing unit tests. It does add overhead per feature we develop, but I think it's worth it for future maintainability of the system. Hopefully, additional tests can follow the same patterns, and require less lift than this one did.

@jfoo1984 jfoo1984 merged commit ea5bc75 into main Jan 27, 2024
5 checks passed
@jfoo1984 jfoo1984 deleted the CZID-9015-add-snapshot-tests branch January 27, 2024 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants