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

[JENKINS-70464] Improve AddEmbeddableBadgeConfigStepTest test coverage #288

Merged
merged 2 commits into from
Jan 30, 2024

Conversation

abhishekmaity
Copy link
Contributor

In response to #114

Testing done

  • Increased the test coverage of existing AddEmbeddableBadgeConfigStepTest.java
  • Improved test coverage shown below

Screenshot 2024-01-30 013946

Submitter checklist

@abhishekmaity abhishekmaity requested a review from a team as a code owner January 29, 2024 20:14
@github-actions github-actions bot added the tests Automated test addition or improvement label Jan 29, 2024
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.

Thanks for the proposal. Two suggestions that are optional. The suggestions don't improve the tests but rather improve the output in case of a failure. The suggestions are optional. If you'd rather not explore Hamcrest assertions in this pull request, that is no problem. I am happy to merge this pull request as it exists now.

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Jan 29, 2024

@abhishekmaity I was surprised when I saw that the overall coverage for this pull request was lower than the coverage for the master branch. I think the surprise is highlighting that you may have forgotten to base your new development off the most recent version of the master branch.

The GitHub user interface on your forked repository includes a button that will "Sync" the primary branch of your fork with the primary branch of the upstream repository. It is good to use that button to sync right before you start development work on a new branch so that you create the new branch based on the most recent primary branch ("master") of the upstream repository.

There are other ways to assure that you always start a branch with the most recent upstream master branch. One technique might be:

$ git clone https://github.com/jenkinsci/embeddable-build-status-plugin.git
$ cd embeddable-build-status-plugin
$ gh repo set-default
$ gh repo fork
! MarkEWaite/embeddable-build-status-plugin already exists
? Would you like to add a remote for the fork? Yes
✓ Added remote origin
$ git pull --all
$ git checkout -b my-new-thing upstream/master

That command sequence assumes that you've installed the GitHub command line utility. I find it to be very helpful.

@MarkEWaite
Copy link
Contributor

I pushed a merge from the upstream master branch to this branch so that the latest test improvements on the master branch would be included in this branch. Refer to f6958bc for the merge

@abhishekmaity
Copy link
Contributor Author

Thanks for the proposal. Two suggestions that are optional. The suggestions don't improve the tests but rather improve the output in case of a failure. The suggestions are optional. If you'd rather not explore Hamcrest assertions in this pull request, that is no problem. I am happy to merge this pull request as it exists now.

@MarkEWaite I would like to explore Hamcrest assertion for test cases next time keeping the valuable advantages over assertTrue . I did not know about these so I would like to explore little more before applying.

@MarkEWaite MarkEWaite merged commit 44a466e into jenkinsci:master Jan 30, 2024
17 checks passed
@abhishekmaity
Copy link
Contributor Author

@abhishekmaity I was surprised when I saw that the overall coverage for this pull request was lower than the coverage for the master branch. I think the surprise is highlighting that you may have forgotten to base your new development off the most recent version of the master branch.

The GitHub user interface on your forked repository includes a button that will "Sync" the primary branch of your fork with the primary branch of the upstream repository. It is good to use that button to sync right before you start development work on a new branch so that you create the new branch based on the most recent primary branch ("master") of the upstream repository.

There are other ways to assure that you always start a branch with the most recent upstream master branch. One technique might be:

$ git clone https://github.com/jenkinsci/embeddable-build-status-plugin.git
$ cd embeddable-build-status-plugin
$ gh repo set-default
$ gh repo fork
! MarkEWaite/embeddable-build-status-plugin already exists
? Would you like to add a remote for the fork? Yes
✓ Added remote origin
$ git pull --all
$ git checkout -b my-new-thing upstream/master

That command sequence assumes that you've installed the GitHub command line utility. I find it to be very helpful.

Thank you for reviewing and pointing this small thing that makes a big impact on performance. In hurry, I skipped the order may be. Will keep in mind whenever committing not only in this project but for any other git related project.

@abhishekmaity abhishekmaity deleted the test-case-improve-2 branch January 30, 2024 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Automated test addition or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants