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

[GSoC] Add UserIdentityTest to improve code coverage in git-plugin #851

Merged
merged 3 commits into from
Mar 9, 2020

Conversation

rishabhBudhouliya
Copy link
Contributor

The unit test was added in relevance to one the project goals of Git-Plugin Performance Improvements which is to improve the code coverage.

Screenshot 2020-03-06 at 3 16 47 AM

I ran the coverage report on IntelliJ and most of the report is not relevant to the test, I have just taken out the relevant part.

Further comments

While writing tests for this class, UserIdentity, I came upon an issue with this extension, namely JENKINS-46052.

While UserIdentity sets author name and email in the environment variable, wouldn't it be right to set the new author name and email in the git.config for that particular repository as well?

git config user.name "GIT_AUTHOR_NAME"
git config user.email "GIT_AUTHOR_EMAIL"

Seems like a small fix, if needed.

@MarkEWaite MarkEWaite added the test label Mar 6, 2020
@rishabhBudhouliya
Copy link
Contributor Author

Thanks for reviewing this despite being busy, really appreciate it.

I wasn't aware of the @WithoutJenkins annotation, it sure is a good way to reduce the cost associated to a JenkinsRule.

I had to shift the creation of TestGitRepo and the GitClient to the particular test which requires JenkinsRule inorder to run one test with JenkinsRule and two without it. The before() runs before each test, hence was not letting the other two run without it.

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Mar 6, 2020

When I merge this commit, I will likely use "Rebase and merge" rather than "Merge" because the commit on which it is based is quite a few commits behind the current master branch.

You may want to investigate the hub command for convenience. It includes a command hub sync which will update your local branches (like master) with the latest results from the upstream repositories. It also has a very nice command hub pr checkout nnn which will checkout a branch for the pull request nnn.

Please don't rebase the commit yourself because that damages the GitHub comments. Better to let GitHub do the rebase.

Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Will merge next week when I'm home from traveling.

@rishabhBudhouliya
Copy link
Contributor Author

After your comment I just saw that my fork was 91 commits behind the jenkinsci git-plugin repo.
I'll definitely look into hub because I have tried merging the original repo into my forked repo just now.

In the further comments section, I have discussed the need of changing the implementation of UserIdentity. Maybe we can discuss it after you come back, nevertheless thank you so much!

@MarkEWaite MarkEWaite merged commit a1c761b into jenkinsci:master Mar 9, 2020
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