[doctor command] Add tests for androidHomeEnvVariable #728
Conversation
Move envinfo to a tool
|
Looks like E2E tests failing because of Azure. |
|
@lucasbento please take a look if you have time 😄 |
lucasbento
left a comment
lucasbento
left a comment
There was a problem hiding this comment.
Looks very good, thanks for this!
Just left a few comments.
packages/cli/src/commands/doctor/healthchecks/__tests__/androidHomeEnvVariable.test.ts
Show resolved
Hide resolved
| }); | ||
|
|
||
| it('logs manual installation steps to the screen', async () => { | ||
| // @ts-ignore |
There was a problem hiding this comment.
Because I'm forcing loader to be of type Ora, but I only put the info property, not the rest.
Though I changed it to any to avoid the // @ts-ignore.
There was a problem hiding this comment.
You can use NoopLoader (grep for it)
There was a problem hiding this comment.
I need to watch it though, so I need to use jest.fn(). Though using NoopLoader, I could use jest.spyOn(). What do you reckon is the best?
There was a problem hiding this comment.
Generate the mock: https://jestjs.io/docs/en/jest-object#jestgenmockfrommodulemodulename
There was a problem hiding this comment.
Or use jest.spyOn, maybe that's even better
There was a problem hiding this comment.
Actually my mistake, I don't need to watch it, so I used the NoopLoader. Thanks @thymikee!
packages/cli/src/commands/doctor/healthchecks/__tests__/androidHomeEnvVariable.test.ts
Outdated
Show resolved
Hide resolved
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Almost there! Thanks for sending tests <3
| {json: true, showNotFound: true}, | ||
| ), | ||
| ); | ||
| const environmentInfo = JSON.parse(await getEnvironmentInfo()); |
There was a problem hiding this comment.
Please move JSON.parse to getEnvironmentInfo
| printCategory({ | ||
| ...issueCategory, | ||
| key, | ||
| }); |
There was a problem hiding this comment.
Can we revert this change? I like it being a one-liner :)
There was a problem hiding this comment.
Yeah sorry, my VSCode formatter settings changed for some reason. Reverted :)
| import {EnvironmentInfo} from '../../types'; | ||
| import {NoopLoader} from '../../../../tools/loader'; | ||
|
|
||
| jest.mock('../common'); |
There was a problem hiding this comment.
Are you mocking common just to spy on logManualInstallation? This likely unnecessarily lowers the coverage. Please use:
import * as common from '../common';
const logSpy = jest.spyOn(common, 'logManualInstallation');| expect(logManualInstallation).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| afterEach(() => { |
There was a problem hiding this comment.
Please move afterEach to the top of describe, this way it's easier to spot the side effects that tests rely on
|
All fixed @thymikee, thanks for the review and the good tips on Jest best practices 😄 |
| const environmentInfo: EnvironmentInfo = JSON.parse( | ||
| await getEnvironmentInfo(), | ||
| ); | ||
| const environmentInfo: EnvironmentInfo = await getEnvironmentInfo(); |
There was a problem hiding this comment.
you likely don't need to cast the type now :)
| json: true, | ||
| showNotFound: true, | ||
| }, | ||
| return JSON.parse( |
There was a problem hiding this comment.
Please cast to EnvironmentInfo
3 participants
Summary:
Started writing tests for the
doctorhealthchecks, starting withandroidHomeEnvVariable.Since I needed the
envinfooutput in the test, and I will need it in all the upcomming tests, I moved it to thetoolsfolder. Maybe it's not the best place, I'm not sure.I only wrote one test to have feedback about the way of writing them, feel free to comment.
IMHO since these are unit tests we should mock all the imports and just make sure we call them when expected.
I didn't test the hard-coded SO links because it seems like a detail that wasn't worth testing.
Related to #694
Test Plan:
This is a test so... CI is green? ✅