jenkinsci / build-timeout-plugin

Jenkins build-timeout plugin
https://plugins.jenkins.io/build-timeout/
31 stars 80 forks source link

Migration from Junit 4 to Junit 5 for Non-integration Tests #88

Closed baruKreddy closed 2 years ago

baruKreddy commented 2 years ago

Migrate non-integration tests to Junit 5

krisstern commented 2 years ago

Hi @baruKreddy for the PR! Just wanted to check with you to see if you have done the testing locally using either mvn test or mvn verify? I am asking because I noticed there are some tests failures and would like to work these through with you before a review can be made for the PR.

baruKreddy commented 2 years ago

@krisstern I have retriggered now with space removal , before all the test case were running successfully locally . Adding screenshot for reference.

image
krisstern commented 2 years ago

@baruKreddy That's good to know. Let me take the time to review the code in more detail over the next few days before processing it further. Appreciate your contribution!

krisstern commented 2 years ago

I have taken the liberty to update the description of the PR as well, and have used the Fixes #85 syntax so that once this PR is merged the corresponding issue will be automatically close as well.

krisstern commented 2 years ago

As @baruKreddy deems the PR incomplete and is still working on it I have converted this into a draft.

krisstern commented 2 years ago

@baruKreddy Would you like to try something new by first squashing (more technically rebasing) the commits to one using fixup and also reword the first commit message at the same time using reword to something like "Migrate non-integration tests to Junit 5"? I can walk you through the entire process if you are unsure about what to do. That way I could have a cleaner history for your PR.

krisstern commented 2 years ago

So the steps are as follows:

  1. At the terminal run git rebase -i HEAD~4 where i in this context stands for the interactive mode. The number is 4 because you have exactly 4 commits in this PR at the moment.
  2. Edit the details via your default editor, change for the verb for the first commit to reword, and that for the other three to fixup.
  3. Save your changes.
  4. Close your editor.
  5. Change your commit message for the first commit.
  6. Save your changes.
  7. Close your editor.
  8. A line will appear in the terminal telling you that you have successfully rebased.
  9. Push the new changes to repo by force using something like git push -f baruKreddy Junit5/feature.
baruKreddy commented 2 years ago

@baruKreddy Would you like to try something new by first squashing (more technically rebasing) the commits to one using fixup and also reword the first commit message at the same time using reword to something like "Migrate non-integration tests to Junit 5"? I can walk you through the entire process if you are unsure about what to do. That way I could have a cleaner history for your PR.

Do I need to change title ("Migration from Junit 4 to Junit 5 for Non-integration Tests") or first comment message "This PR has changes required for migrating from Junit4 to Junit5". I have made changes in the class ElasticTimeOutStrategyTest.java. If this changes are correct will go on with the same for remaining classes.)

krisstern commented 2 years ago

You only need to change the first because when you use fixup the other commits their messages will be sort of "erased".

krisstern commented 2 years ago

I have already changed the title of this PR for you so you do not need to change it further.

krisstern commented 2 years ago

Please use "Migrate non-integration tests to Junit 5" for the new first commit message though because if the message is too long there may be display issues.

baruKreddy commented 2 years ago

I have updated the message, please give a look!

baruKreddy commented 2 years ago

So the steps are as follows:

  1. At the terminal run git rebase -i HEAD~4 where i in this context stands for the interactive mode. The number is 4 because you have exactly 4 commits in this PR at the moment.
  2. Edit the details via your default editor, change for the verb for the first commit to reword, and that for the other three to fixup.
  3. Save your changes.
  4. Close your editor.
  5. Change your commit message for the first commit.
  6. Save your changes.
  7. Close your editor.
  8. A line will appear in the terminal telling you that you have successfully rebased.
  9. Push the new changes to repo by force using something like git push -f baruKreddy Junit5/feature.

Let me go ahead with this instructions, once done will update you

baruKreddy commented 2 years ago

@krisstern I have made changes according to your comments. Please go verify it.

krisstern commented 2 years ago

@baruKreddy I think you did squash instead of fixup, something looks off to me?

krisstern commented 2 years ago

The multiline commit message just looks a bit odd to me... Did you make sure to comment out the lines "Junit5 Migration" and "Update ElasticTimeOutStrategyTest.java" below "Migrate non-integration tests to Junit 5" with a hash # symbol? We may need to do another reword again to fix this.

krisstern commented 2 years ago

This is what I am referring to:

Screenshot 2022-03-09 at 11 04 15 PM
baruKreddy commented 2 years ago

The multiline commit message just looks a bit odd to me... Did you make sure to comment out the lines "Junit5 Migration" and "Update ElasticTimeOutStrategyTest.java" below "Migrate non-integration tests to Junit 5" with a hash # symbol? We may need to do another reword again to fix this.

Should i just keep Migrate non-integration tests to Junit 5 and remove other comments or just comment?

krisstern commented 2 years ago

Should i just keep Migrate non-integration tests to Junit 5 and remove other comments or just comment?

Yes, keep the first line and comment out all other lines for the commit message.

baruKreddy commented 2 years ago

Now it's all sorted, just one message ("Migrate non-integration tests to Junit 5") and one commit. Just give a look!

krisstern commented 2 years ago

Looks awesome now. Great job! I will review the PR over the next few days.

baruKreddy commented 2 years ago

Thanks @krisstern! :)