-
Notifications
You must be signed in to change notification settings - Fork 282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve test coverage of onefetch
#700
Comments
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. |
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. |
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). |
onefetch
onefetch
* Adding test coverage to src/info/info_field.rs As requested as an item on task #700 * Implementing the suggested changes by reviewer
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? |
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". |
As for integration tests, please make sure not to rely on the For pointers on how to do it, I suggest you take a look at @Byron comment --> #705 (comment) |
In integration tests, I think there shouldn't be any need for either |
Trying to test New directory As a novice this seems pretty "doable" approach, for what that is worth :-) |
I took a look and my answer is "yes, well done :)".
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
After leaving a few single-line comments directly on the commit, is there any reason this isn't a draft PR yet? |
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. |
PR #822 contains the bare git repo test, another use of git-testtools. |
* testing get_git_username using git-testtools #700 * testing title format function * oops, fixing code format * adding more testing to get better coverage * combining basic git repo create functions and renaming script * fixing format error * Update src/info/title.rs Co-authored-by: Spenser Black <[email protected]> * Update src/info/title.rs Co-authored-by: Spenser Black <[email protected]> * Update src/info/title.rs Co-authored-by: Spenser Black <[email protected]> Co-authored-by: Spenser Black <[email protected]>
Signed-off-by: gallottino <[email protected]> Co-authored-by: Oniryu95 <[email protected]>
Signed-off-by: gallottino <[email protected]> Co-authored-by: Oniryu95 <[email protected]>
Signed-off-by: gallottino <[email protected]> Co-authored-by: Oniryu95 <[email protected]>
Signed-off-by: gallottino <[email protected]> Co-authored-by: Oniryu95 <[email protected]>
Signed-off-by: gallottino <[email protected]> Co-authored-by: Oniryu95 <[email protected]> Signed-off-by: gallottino <[email protected]> Co-authored-by: Oniryu95 <[email protected]>
* testing bare git repo error using git-testtools script #700 * after running cargo fmt * refactoring new to call init for testing of bare repo detection * brute force testing of format function * idea for testing formatted output using git-testtools arguments * fix formatting * adding pretty assert and concept of match anything in expected string * fixing windows expected output failures * fixing formatting, eventually I will use a hook * removing color based checking, simple regex now * Update src/info/mod.rs Co-authored-by: Spenser Black <[email protected]> * Update src/info/mod.rs Co-authored-by: Spenser Black <[email protected]> * updates to use external file w/ regex in it * trying standard path separator to see if windows ci passes * updates to regex that make it easier to read * test coverage of serializer using json * fix formatting * better assert logic for bare repo and pretty assert to debug CI failure in json * fixing git version in json serializarion * obtains 100% code coverage by manipulating git repo and files parsed * more code coverage, trying to get those last few lines * making use of snapshot name in testing * changing verification to string based test * switching to using inst snapshots * adding snap file needed for previous commit * refactor default into Config object to allow Default construction * better naming on function * removing snap file from ignore * using object serialization in test Co-authored-by: Spenser Black <[email protected]>
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
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:
The text was updated successfully, but these errors were encountered: