Skip to content
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

Open
22 of 32 tasks
o2sh opened this issue Jul 9, 2022 · 15 comments
Open
22 of 32 tasks

Improve test coverage of onefetch #700

o2sh opened this issue Jul 9, 2022 · 15 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Milestone

Comments

@o2sh
Copy link
Owner

o2sh commented Jul 9, 2022

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

@o2sh o2sh added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Jul 9, 2022
@o2sh o2sh added this to the v2.13.0 milestone Jul 9, 2022
@o2sh o2sh added pinned and removed pinned labels Jul 9, 2022
@o2sh o2sh pinned this issue Jul 9, 2022
@spenserblack
Copy link
Collaborator

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
Copy link
Owner Author

o2sh commented Jul 12, 2022

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
Copy link
Collaborator

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
Copy link
Owner Author

o2sh commented Jul 12, 2022

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

codecov

@o2sh o2sh mentioned this issue Jul 24, 2022
2 tasks
o2sh added a commit that referenced this issue Aug 6, 2022
o2sh added a commit that referenced this issue Aug 6, 2022
o2sh added a commit that referenced this issue Aug 7, 2022
o2sh added a commit that referenced this issue Aug 7, 2022
o2sh added a commit that referenced this issue Aug 13, 2022
o2sh added a commit that referenced this issue Aug 14, 2022
o2sh added a commit that referenced this issue Aug 15, 2022
o2sh added a commit that referenced this issue Aug 15, 2022
o2sh added a commit that referenced this issue Aug 15, 2022
o2sh added a commit that referenced this issue Aug 15, 2022
o2sh added a commit that referenced this issue Aug 15, 2022
o2sh added a commit that referenced this issue Aug 15, 2022
@o2sh o2sh changed the title Improve test coverage in onefetch Improve test coverage of onefetch Sep 10, 2022
o2sh pushed a commit that referenced this issue Oct 7, 2022
* Adding test coverage to src/info/info_field.rs

As requested as an item on task #700

* Implementing the suggested changes by reviewer
@alessandroasm
Copy link
Contributor

alessandroasm commented Oct 8, 2022

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
Copy link
Collaborator

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
Copy link
Owner Author

o2sh commented Oct 9, 2022

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 --> #705 (comment)

@Byron
Copy link
Collaborator

Byron commented Oct 9, 2022

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
Copy link
Contributor

atluft commented Oct 9, 2022

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
Copy link
Collaborator

Byron commented Oct 10, 2022

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
Copy link
Contributor

atluft commented Oct 10, 2022

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
Copy link
Collaborator

Byron commented Oct 10, 2022

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
Copy link
Contributor

atluft commented Oct 12, 2022

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

o2sh pushed a commit that referenced this issue Oct 14, 2022
* 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]>
gallottino added a commit to gallottino/onefetch that referenced this issue Oct 18, 2022
Signed-off-by: gallottino <[email protected]>
Co-authored-by: Oniryu95  <[email protected]>
gallottino added a commit to gallottino/onefetch that referenced this issue Oct 18, 2022
Signed-off-by: gallottino <[email protected]>
Co-authored-by: Oniryu95  <[email protected]>
gallottino added a commit to gallottino/onefetch that referenced this issue Oct 18, 2022
Signed-off-by: gallottino <[email protected]>
Co-authored-by: Oniryu95  <[email protected]>
gallottino added a commit to gallottino/onefetch that referenced this issue Oct 18, 2022
Signed-off-by: gallottino <[email protected]>
Co-authored-by: Oniryu95  <[email protected]>
o2sh pushed a commit that referenced this issue Oct 19, 2022
Signed-off-by: gallottino <[email protected]>
Co-authored-by: Oniryu95  <[email protected]>

Signed-off-by: gallottino <[email protected]>
Co-authored-by: Oniryu95  <[email protected]>
o2sh pushed a commit that referenced this issue Oct 29, 2022
* 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]>
@o2sh
Copy link
Owner Author

o2sh commented Mar 12, 2023

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
Copy link
Owner Author

o2sh commented Oct 1, 2023

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 o2sh added the stale label Oct 1, 2023
@o2sh o2sh removed the stale label Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants