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

testing get_git_username using git-testtools for #700 #812

Merged
merged 9 commits into from
Oct 14, 2022
Merged

testing get_git_username using git-testtools for #700 #812

merged 9 commits into from
Oct 14, 2022

Conversation

atluft
Copy link
Contributor

@atluft atluft commented Oct 9, 2022

Trying to add a test for a function that uses a git repo created specifically to test the function. In this case the repo has a local configuration w/ the "committer.name" and "committer.email" set to known values.

Other typical values are added to the repo too. Like "user.name" and "user.email" and "author.name" and "author.email" to ensure those aren't selected as the correct values, although there are cases where those would be correct.

Interesting discovery is that "committer.name" won't be selected if "committer.email" isn't present too. Not sure that is an issue to pursue or not.

Testing:

cargo test -- --nocapture

@atluft
Copy link
Contributor Author

atluft commented Oct 9, 2022

Hi @o2sh and @spenserblack,
Please advise about the code coverage failures below, is the new test failing or is it that code coverage went negative by adding more code into measurement, or something else :-) ?

@o2sh
Copy link
Owner

o2sh commented Oct 10, 2022

I suspect a recent update of tarpaulin to be the cause for the drop:

image

Indeed, we always pull the latest version when generating the codecov report.

- name: Install cargo-tarpaulin
  uses: actions-rs/install@v0.1
  with:
    crate: cargo-tarpaulin
    version: latest 

This is just a supposition, I haven't tested it locally.

@@ -1,83 +1,155 @@
use super::get_style;
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 Looks like this file was (correctly) converted to LF line endings.
For the sake of a reasonable diff, I think we should convert all files to LF so that it's easier to compare this and #813

Copy link
Contributor Author

@atluft atluft Oct 10, 2022

Choose a reason for hiding this comment

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

I see working on converting in #815
Oops, did I do that? Shoot, I really was trying to keep it CR LF, I guess 2 wrongs do make a right!
It is crazy that in 2022 we have to think about line endings, maybe in another 100 years this will be resolved.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, did I do that?

You correctly did that (See .editorconfig file) 😆

I incorrectly set up an EditorConfig and never enforced its settings in a workflow.

o2sh added a commit that referenced this pull request Oct 10, 2022
* Convert line endings to LF

This should result in more reasonable diffs of future contributions
where a contributor's text editor would automatically convert line
endings.

See #812, #813

* Create .rustfmt.toml

Enforces Unix-style newlines.

Co-authored-by: Ossama Hjaji <ossama-hjaji@live.fr>

Co-authored-by: Ossama Hjaji <ossama-hjaji@live.fr>
@spenserblack
Copy link
Collaborator

Thanks @atluft! If you feel comfortable rebasing onto main then can you please do so?
There's a good chance that this might be squash-merged, so a normal merge commit is fine, too.

git checkout --ours and git checkout --theirs are useful when resolving these full-file conflicts.

@atluft
Copy link
Contributor Author

atluft commented Oct 11, 2022

@spenserblack
I think this PR is in good shape now.
git rebase main, correct conflicts, continue, push force at end.

squash is a wonderful feature!

@atluft atluft marked this pull request as ready for review October 11, 2022 22:13
@atluft
Copy link
Contributor Author

atluft commented Oct 11, 2022

Discovered the "Ready for Review" button!

src/info/title.rs Outdated Show resolved Hide resolved
src/info/title.rs Show resolved Hide resolved
Co-authored-by: Spenser Black <spenserblack01@gmail.com>
Copy link
Collaborator

@spenserblack spenserblack left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for helping us with testing! Leaving one minor nitpick.

src/info/title.rs Outdated Show resolved Hide resolved
src/info/title.rs Outdated Show resolved Hide resolved
atluft and others added 2 commits October 13, 2022 22:04
Co-authored-by: Spenser Black <spenserblack01@gmail.com>
Co-authored-by: Spenser Black <spenserblack01@gmail.com>
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.

None yet

3 participants