jenkinsci / embeddable-build-status-plugin

Embed build status of Jenkins jobs in web pages
https://plugins.jenkins.io/embeddable-build-status/
MIT License
166 stars 259 forks source link

[JENKINS-70464] Improve `AddEmbeddableBadgeConfigStepTest` test coverage #288

Closed abhishekmaity closed 7 months ago

abhishekmaity commented 7 months ago

In response to #114

Testing done

Screenshot 2024-01-30 013946

### Submitter checklist
- [x] Make sure you are opening from a **topic/feature/bugfix branch** (right side) and not your main branch!
- [x] Ensure that the pull request title represents the desired changelog entry
- [x] Please describe what you did
- [x] Link to relevant issues in GitHub or Jira
- [x] Link to relevant pull requests, esp. upstream and downstream changes
- [x] Ensure you have provided tests - that demonstrates feature works or fixes the issue
MarkEWaite commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

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.

abhishekmaity commented 7 months ago

@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.