Skip to content

refactor: use more production code, fewer mocks in github tests#1943

Open
ckerr wants to merge 13 commits into
mainfrom
refactor/fewer-mocks-in-github-tests
Open

refactor: use more production code, fewer mocks in github tests#1943
ckerr wants to merge 13 commits into
mainfrom
refactor/fewer-mocks-in-github-tests

Conversation

@ckerr
Copy link
Copy Markdown
Member

@ckerr ckerr commented May 9, 2026

Fifth in a series to move GitHub token + gist management into the main process.

This is a test cleanup PR:

  • Adds support for app.setPath() in the Electron mock
  • Throws an error if the Electron mock's app.getPath() is called with an unexpected key.
  • Makes STATIC_DIR in main/constants work in both production and test environments
  • In github tests, remove the mocks for
    • node:fs
    • fs-extra
    • getTemplate()
  • in github tests, try to not rely on implementation details, eg not mocking/spying low-level filesystem calls. The tests shouldn't care which of fs.existsSync() or fsPromises.exists() is called by production code.

@ckerr ckerr requested a review from dsanders11 May 9, 2026 14:34
@ckerr ckerr requested review from a team and codebytere as code owners May 9, 2026 14:34
@coveralls
Copy link
Copy Markdown

coveralls commented May 9, 2026

Coverage Status

coverage: 88.535% (+0.07%) from 88.468% — refactor/fewer-mocks-in-github-tests into main

@ckerr ckerr removed the request for review from dsanders11 May 9, 2026 14:51
@ckerr ckerr changed the title refactor: use more production code, fewer mocks in github tests [DRAFT] refactor: use more production code, fewer mocks in github tests May 9, 2026
@ckerr ckerr force-pushed the refactor/fewer-mocks-in-github-tests branch from 3ef7374 to 1cca7f5 Compare May 9, 2026 17:12
@ckerr ckerr changed the title [DRAFT] refactor: use more production code, fewer mocks in github tests refactor: use more production code, fewer mocks in github tests May 9, 2026
@ckerr ckerr requested a review from dsanders11 May 9, 2026 17:18
Copy link
Copy Markdown
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

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.

Comment thread src/main/constants.ts Outdated
Comment thread tests/main/github.spec.ts Outdated
mockOctokitInstance();
expect(loadToken()).toBeNull();

const expectedSignInResult = { success: true, login: MOCK_LOGIN };
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There are warnings that this is unused.

Copy link
Copy Markdown
Member Author

@ckerr ckerr May 13, 2026

Choose a reason for hiding this comment

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

Fixed in 5990348

@ckerr
Copy link
Copy Markdown
Member Author

ckerr commented May 13, 2026

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.

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.

I've added a safeguard in the beforeEach() call in 8e66d1d to confirm that the credentials file is inside of our tmpdir. This should prevent the github tests from doing any mutating calls elsewhere.

There are other tests in the file that read from the static template directory, but those are read-only.

…anging

fix: oops, removed a line accidentally
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants