o2sh / onefetch

Command-line Git information tool
https://onefetch.dev
MIT License
10.32k stars 277 forks source link

Improve test coverage of `onefetch` #700

Open o2sh opened 2 years ago

o2sh commented 2 years ago

We're lacking both integration and unit tests to make sure we won't run into regressions after new development/refactoring.

Modules that should be covered by unit tests:

UI

Image backends

Info

Repo info fields

Dependencies (package manager)

Language

Modules that should be covered by integration tests:


codecov

spenserblack commented 2 years ago

BTW I've used codecov.io on a few projects. I like its visualizations, and its GitHub integration can add checks to commits and PRs for coverage deltas.

o2sh commented 2 years ago

Following your suggestion, I plugged the repo to codecov with a ci job that will generate the coverage report (with cargo tarpaulin) and upload it to codecov automatically.

👉 https://app.codecov.io/gh/o2sh/onefetch

spenserblack commented 2 years ago

Great! Would you want to add a coverage badge? The markdown can be found in the settings page (I don't have access to the settings).

o2sh commented 2 years ago

it's very low...maybe we should wait before putting it on display 😅

codecov

alessandroasm commented 2 years ago

Hello @o2sh and @spenserblack! I was wondering on how to test the git integration in the mentioned files. I see two possible solutions: using mockall crate or testing this operations on simple temporary git repos. The latter approach is used by gitui project; here is the function they use to initilize the repo: https://github.com/extrawurst/gitui/blob/986d34a5acd520fbec91386675bec8013affc6bd/asyncgit/src/sync/mod.rs#L212-L255

And here is a file using that for unit tests: https://github.com/extrawurst/gitui/blob/006cdd63738db91e6c3074439bfd561eb0b5eb9c/asyncgit/src/sync/stash.rs#L153

What do you think about this? Thank you

spenserblack commented 2 years ago

To be honest I like both approaches.

For unit tests, I think mocking is a great idea, but for integration tests I like creating temporary git repos.

I like mocking especially when interacting with a library that can have many reasons to return an error. It allows the test to be "if we receive x response, we should do y" instead of "in this specific state, we should receive x response, and then do y."

In other words, I like making small unit tests for "do we handle this git library's responses correctly" and larger integration tests for "do we handle this git repository correctly".

o2sh commented 2 years ago

As for integration tests, please make sure not to rely on the git2 crate as we are trying to get rid of it in favor of gitoxide

For pointers on how to do it, I suggest you take a look at @Byron comment --> https://github.com/o2sh/onefetch/pull/705#issuecomment-1193484501

Byron commented 2 years ago

In integration tests, I think there shouldn't be any need for either git2 or gitoxide if git-testtools is used. The latter allows to write shell scripts which invoke git to build a fixture while dealing with all the nitty and the gritty for you, while assuring it runs fast locally and on CI if git-lfs with the fixture-archive feature was to be used. I am saying this knowing that if fixtures were to be built with gitoxide programmatically (as opposed to in sh with git), you would quickly run into limitations with some likely to persist to early next year.

atluft commented 2 years ago

Trying to test get_git_username function in title.rs. Is it close to what is expected?

New directory test/fixture holds the script to create the git repo for testing. Notice the generated git repos are in the source tree. I need help if those should be moved somewhere else. To address the generated code in the source tree I modified .gitignore file to ignore *generated* dirs.

As a novice this seems pretty "doable" approach, for what that is worth :-)

Byron commented 2 years ago

Is it close to what is expected?

I took a look and my answer is "yes, well done :)".

Notice the generated git repos are in the source tree.

That's intentional as it makes it simpler to debug scripts while writing them or play around with those fixtures by hand. Moving them out of the source tree isn't supported ATM, and using .gitignore is the way to avoid accidents. You will see that these directories are never deleted either and are named after a hash of the contents of the generating script.

As a novice this seems pretty "doable" approach, for what that is worth :-) 🎉🙏

After leaving a few single-line comments directly on the commit, is there any reason this isn't a draft PR yet?

atluft commented 2 years ago

Hi @Byron Thanks for having a look, the comments look fair and reasonable. End of my day here, will work thru this week. Never used a draft PR before, see PR #812 marked it as a draft.

Byron commented 2 years ago

Ah, #812 was what I was looking for, thank you! The draft status doesn't matter, I assumed no PR was available as development was still early, and the draft status helps to signal a review isn't needed yet. I guess it would be nice if GitHub would make it easier to get from a commit listed in this issue to the corresponding PR.

atluft commented 2 years ago

PR #822 contains the bare git repo test, another use of git-testtools.

o2sh commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

o2sh commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.