Tests for google-sheets.ts #49
Conversation
src/libs/google-sheets.test.ts
Outdated
|
|
||
| describe('getAuthClient', () => { | ||
| it('should successfully create and authorize a Google Sheets API client', async () => { | ||
| (google.sheets as jest.Mock).mockImplementationOnce( |
There was a problem hiding this comment.
I thought we had already mocked this on L24 above?
src/libs/google-sheets.test.ts
Outdated
| expect(google.auth.JWT).toHaveBeenCalledWith( | ||
| process.env.CLIENT_EMAIL, | ||
| null, | ||
| process.env.GOOGLE_PRIVATE_KEY.replace(/\\n/g, '\n'), |
There was a problem hiding this comment.
One thing we could do to test this key logic properly is to have the test value in the env set including test-key\\n and then see if we are calling this JWT with just test-key\n. Then we are testing the desired behavior (removing an extra slash) more so than the implementation logic (using replace)
src/libs/google-sheets.test.ts
Outdated
| '$description', | ||
| async ({ n, expectedRange, expectedComments }) => { | ||
| MOCK_SHEETS.spreadsheets.values.get | ||
| .mockResolvedValueOnce({ data: { values: [['3']] } }) // calls getTotalRows |
There was a problem hiding this comment.
Wondering if it makes more sense to mock getTotalRows here. That way, the dev doesn't need to know the order in which we are calling spreadsheets.values.get in the getLastNComments function in order to mock properly. Could imagine us adding another call to sheets in the function that would then mess up the logic here. We could separately test getTotalRows in a future PR
There was a problem hiding this comment.
I ended up going this route since getTotalRows isn't exported from google-sheets.ts (and I didn't think I should export it for the sake of testing). Any recs on how to achieve this?
There was a problem hiding this comment.
Hmm good point. I personally feel like we're better off exporting this so it can be properly unit tested. I feel like we get more value from having a unit test to increase coverage vs. what we lose in exposing this function to other modules via export. There could be a best of both worlds solution that I am missing, though. Maybe a good question to ask on the engineering channel!
|
Making a note that it could be good to do a check for |
Added tests to the functions that are used by summarize handler for now, can add on tests to the other functions later but wanted to try to move forward with updating summarize so pushing this out for now. Also updated getTotalRows functionality to handle when the spreadsheet has less comments than requested.