Test getClient #509
Test getClient #509
Conversation
chadwhitacre
force-pushed
the
cwlw/test-getclient
branch
from
88c99f8 to
bc80527
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #509 +/- ##
=======================================
Coverage ? 84.47%
=======================================
Files ? 97
Lines ? 2538
Branches ? 496
=======================================
Hits ? 2144
Misses ? 387
Partials ? 7
☔ View full report in Codecov by Sentry. |
| @@ -1,23 +1,16 @@ | |||
| # direnv will make a copy of this file as an .env file | |||
| # Follow the instructions in README.md as how to add your personal | |||
| # tokens and secrets in this file | |||
There was a problem hiding this comment.
We don't need all this boilerplate in the test env, it's copy-pasta from the example env.
|
|
||
| # GitHub App ID | ||
| # GitHub | ||
| FORCE_USER_TOKEN_GITHUB_CLIENT="false" |
There was a problem hiding this comment.
Bringing this up here as it belongs with GitHub config.
| # NOTE: this *must* be on a single line, otherwise it will break | ||
| # reading the secret key. | ||
| GH_APP_SECRET_KEY='-----BEGIN RSA PRIVATE KEY----------END RSA PRIVATE KEY-----' | ||
| GH_APP_SECRET_KEY="top \nsecret\n key" |
There was a problem hiding this comment.
Switching to a more test-friendly value, also exercising the \n-stripping behavior.
| OctokitWithRetries.apps = { | ||
| getOrgInstallation: jest.fn(async ({ org }) => { | ||
| return { | ||
| data: { | ||
| id: `installation-${org}` | ||
| } | ||
| }; | ||
| }), | ||
| }; |
There was a problem hiding this comment.
This part is new, it's needed now since we call this inside getClient.
| }; | ||
|
|
||
| OctokitWithRetries.orgs = { | ||
| checkMembershipForUser: jest.fn(async function ({ org, username }) { |
There was a problem hiding this comment.
I took an opportunity to clean up the call signature here a bit, I had been using (x) ... x.org previously. Besides the apps addition above, this is the only other material change to the mock client API, sorry I don't have a clean diff for you.
|
|
||
| const _CLIENTS_BY_ORG = new Map(); | ||
| const OctokitWithRetries = Octokit.plugin(retry); |
There was a problem hiding this comment.
Factoring out for mockability.
| GETSENTRY_REPO: 'getsentry', | ||
| SENTRY_REPO: 'sentry', | ||
| }; | ||
| }); |
There was a problem hiding this comment.
This duplicates the .env.test config and the .env.test config takes precendence, so let's consolidate everything there.
| @@ -1,16 +1,4 @@ | |||
| /* eslint-env jest */ | |||
| jest.mock('@api/slack'); | |||
| jest.mock('@api/github/getClient'); | |||
| jest.mock('@api/github/octokitWithRetries'); | |||
chadwhitacre
force-pushed
the
cwlw/test-getclient
branch
from
bc80527 to
74b497f
Compare
|
Rebased for linting. |
|
|
||
| import { MockOctokitError } from './mockError'; | ||
|
|
||
| // Looks goofy, don't it? This is what I could figure out to mock a class. I |
There was a problem hiding this comment.
What were you aiming for here? I think a manual mock here seems fine as the implementation details are clear and can be used across files
Part of #482, before #503 (came up on #503).
We've been putting off testing
getClient. No longer!The strategy here is to mock at a lower level:
OctokitWithReplies.