refactor: use more production code, fewer mocks in github tests#1943
refactor: use more production code, fewer mocks in github tests#1943ckerr wants to merge 13 commits into
Conversation
Hoist the gist size/count limits out of `src/main/github.ts` into `src/constants.ts` so tests can reference them by name instead of duplicating the magic numbers.
3ef7374 to
1cca7f5
Compare
dsanders11
left a comment
There was a problem hiding this comment.
Left some concrete comments, but I also have a meta comment.
I understand the motivation here of removing some of the implementation details leaking into tests regarding which FS operations are called, but I also have some concerns about unmocking the FS operations and letting tests write to the FS as tests run amock could leave a lot of garbage on disk. These tests are still making an implementation assumption about the userData path being where GitHub tokens are written to, so if that changes then the writes go somewhere other than intended.
Vitest recommends using memfs so that you don't have to mock individual FS calls, but you also don't write to disk. Unfortunately I think memfs has a larger dependency graph than we'd want to pull in at the moment.
So not blocking this PR, but I do wonder if there's a happy middle ground, before we start ripping out all the FS mocks. Perhaps something like an FS mock that wraps the real FS operations but ensures all paths are inside of tmpdir to prevent any unintended FS operations from the tests if we miss something.
| mockOctokitInstance(); | ||
| expect(loadToken()).toBeNull(); | ||
|
|
||
| const expectedSignInResult = { success: true, login: MOCK_LOGIN }; |
There was a problem hiding this comment.
There are warnings that this is unused.
Co-authored-by: David Sanders <dsanders11@ucsbalum.com>
I've added a safeguard in the There are other tests in the file that read from the static template directory. Since those are read-only, there's zero risk to the live filesystem, it exercises more production code, and it simplifies the tests since we don't have to mock the templates, IMO the template part of this is a win. |
…anging fix: oops, removed a line accidentally
|
@dsanders11 are there any other changes you'd like to see? |
Fifth in a series to move GitHub token + gist management into the main process.
This is a test cleanup PR:
app.setPath()in the Electron mockapp.getPath()is called with an unexpected key.STATIC_DIRinmain/constantswork in both production and test environmentsfs.existsSync()orfsPromises.exists()is called by production code.