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-60940: Convert git client plugin tests from JUnit 3 to JUnit 4 #814

Closed
wants to merge 2 commits into from
Closed

JENKINS-60940: Convert git client plugin tests from JUnit 3 to JUnit 4 #814

wants to merge 2 commits into from

Conversation

kdaud
Copy link

@kdaud kdaud commented Feb 2, 2022

JENKINS-60940 - Convert JUnit 3 tests to JUnit 4

Convert JUnit 3 tests to JUnit 4 so that they are easier to maintain and easier to understand.

Checklist

  • I have read the CONTRIBUTING doc
  • I have referenced the Jira issue related to my changes in one or more commit messages
  • I have added tests that verify my changes
  • Unit tests pass locally with my changes
  • I have added documentation as necessary
  • No Javadoc warnings were introduced with my changes
  • No spotbugs warnings were introduced with my changes

Types of changes

  • Tests

@github-actions github-actions bot added the test Automated test addition or improvement label Feb 2, 2022
@MarkEWaite
Copy link
Contributor

@kdaud thanks very much for doing this. Once I've started the review of the changes, please don't use --force to force push changes. When new changes replace existing changes, the comments are no longer associated with the change.

It is better that the changes evolve as a series of small commits and then we can decide before merging if the many small commits should be squashed into a single commit.

@@ -58,7 +58,7 @@
*/

@RunWith(Parameterized.class)
public class GitAPITest {
public abstract class GitAPITest {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather we not change this parameterized test class that checks command line git and two variants of JGit into an abstract class. If you find that you truly need an abstract class, create a new class to receive those tests that need the abstract class.

Copy link
Author

@kdaud kdaud Feb 2, 2022

Choose a reason for hiding this comment

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

Thanks @MarkEWaite for the quick review, my need was to convert this class to an abstract class so that its able to host test methods that consumes abstract methods.

If you find that you truly need an abstract class, create a new class to receive those tests that need the abstract class.

I now have the way to go based on the guidance.

@kdaud
Copy link
Author

kdaud commented Feb 2, 2022

@MarkEWaite have updated the commit accordingly !

@MarkEWaite MarkEWaite self-requested a review February 4, 2022 00:09
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.

@kdaud the change you've proposed removes all the tests from GitAPITestCase and places them in an abstract test class GitAPICaseTest. Because GitAPICaseTest is abstract, none of the tests are executed. That's not what is wanted with the transition from JUnit 3 to JUnit 4.

The instructions in JENKINS-60940 specifically advise that the transformation from JUnit 3 to JUnit 4 be performed one test at a time.

It may be possible to do more conversions in a single step with your abstract test class idea, but if so, then the tests must execute. It is not enough to copy the test source code to a new location that causes them to not be executed.

@MarkEWaite
Copy link
Contributor

I'm closing this in favor of the process started in #815

@MarkEWaite MarkEWaite closed this Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Automated test addition or improvement
Projects
None yet
2 participants