Skip to content

Improve test coverage#711

Open
lysanntranvouez wants to merge 21 commits intomssun:masterfrom
lysanntranvouez:feature/more-tests-pr
Open

Improve test coverage#711
lysanntranvouez wants to merge 21 commits intomssun:masterfrom
lysanntranvouez:feature/more-tests-pr

Conversation

@lysanntranvouez
Copy link
Copy Markdown

I am looking into making some larger changes (#705 in particular...), so I wanted to improve the test coverage first to avoid introducing new issues. Making a separate PR to avoid PR bloat, and because the increased test coverage should hopefully be more valuable than hurt, even if the rest of the work never makes it in for some reason.

The bulk of the changes are unit tests in the relevant areas, and should therefore not affect the functionality of the app.

I have made some small changes to the app code when needed or when I ran into what seemed like bugs while making the tests. Plus small refactors (PersistenceController.init, making some functions private).

One notable change to the tests: I've added some repository fixtures. As in, clonable git repos in passKitTests. They are bare repos, so not obvious to review, unfortunately. But they remove the need to have internet when running the PasswordStoreTest. Let me know if you'd prefer keeping a real world test with the remote GitHub repo.

I recommend to review individual commits.

Copy link
Copy Markdown
Collaborator

@SimplyDanny SimplyDanny left a comment

Choose a reason for hiding this comment

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

More tests are always appreciated. Thanks for that!

As for the bare Git repository: You can also clone from non-bare repos. That might be easier to maintain.

@lysanntranvouez
Copy link
Copy Markdown
Author

lysanntranvouez commented Mar 12, 2026

As for the bare Git repository: You can also clone from non-bare repos. That might be easier to maintain.

Git really doesn't like checking in another git repository inside of it. Except for submodules.
Listing the options I can think of:

  1. Continue using your existing test fixture repo, making several branches for different fixtures. Tests reference that github remote URL.
  2. Continue using your existing test fixture repo, making several branches for different fixtures. They are included as git submodules.
  3. Use the current approach in the PR: check in bare repos
  4. Git repos inside zip files

  1. is really cumbersome for external contributors since they cannot change the remote fixture, try the tests, iterate. So I would really discourage that one. Needs extra PRs.
  2. is still a bit cumbersome due to extra PRs being needed. Adding a new fixture would require making a new branch, which requires write access to the test fixtures repo. Updating it locally is easy though.
  3. makes it cumbersome to inspect the fixtures (requires cloning it). Updating it locally is really easy. No external PRs.
  4. Also not super easy to inspect but a bit easier maybe. Needs a bit more plumbing in the test setup. Iterating on them is a bit more annoying since you need to re-zip. No external PRs.

Which one would you prefer? And let me know if you have any better ideas.

If you go for 1 or 2, can you make some branches please? 😅
Right now locally I have:

  • "with-gpgid" (this is your current "master" branch)
  • "empty"
  • "empty-dirs"

@lysanntranvouez
Copy link
Copy Markdown
Author

I've addressed your comments and hopefully fixed the pipeline.

We should autosquash the fixup commits before merging, they are just a reviewing aid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants