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

FILEUPLOAD-302: JUnit Jupiter migration #23

Merged

Conversation

@mureinik
Copy link
Contributor

mureinik commented Oct 7, 2019

This PR upgrades the project's testing framework from JUnit 4.12 to the modern JUnit Jupiter 5.5.2.

Since JUnit 5 Jupiter is not backwards compatible to JUnit 4.x (or even JUnit Vintage), this PR is a bit large, even though most of the changes are merely cosmetic (such as changing the argument order,
see details below). In order to make the reviewer's task as easy as possible, this PR does not presume to use JUnit Jupiter's best practices and all its new functionality, but only to migrate the
existing tests with as little change as possible. Following PRs may want to improve the tests by using some of JUnit Jupiter's new features.

This PR includes the following changes:

  1. Maven dependency changes:

    1. junit:junit was replaced with org.junit.jupiter:junit-jupiter.
  2. Annotations:

    1. org.junit.jupiter.api.Test was used as a drop in replacement for org.juit.Test without arguments. See 3.i. for handling of @Test annotations with an expected argument.
    2. The missing @Test annotations were added to StreamingTest's test method. JUnit 4 allows omitting them, while JUnit Jupiter does not.
    3. org.junit.jupiter.api.BeforeEach was used as an drop in replacement for org.junit.Before.
    4. org.junit.jupiter.api.AfterEach was used as an drop in replacement for org.junit.After.
  3. Assertions:

    1. org.junit.jupiter.api.Assertions' methods were used as drop in replacements for org.junit.Assert's methods with the same name in the simple case of an assertion without a message. In the case of an assertion with a message, org.junit.jupiter.api.Assertions' methods were used, but the argument order was changed - Assert's methods take the message as the first argument, while Assertions' methods take the message as the last argument.
    2. org.junit.jupiter.api.Assertions' methods were used as drop in replacements for junit.framework.TestCase's methods with the same name in the simple case of an assertion without a message.
    3. org.junit.jupiter.api.Assertions#assertThrows was used to assert that a specific exception was throws instead of an org.junit.Test annotation with an expected argument. As a side bonus, this change makes the tests slightly stricter, as now they can assert the exception was thrown from a specific line and prevent false positives where the test's "set-up" code accidentally threw that exception.
  4. Parameterized tests:

    1. FileUploadTest was rewritten with @ParameterizedTest and @MethodSource in order to gain an equivalent functionality of JUnit 4's Parameterized runner.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 7, 2019

Coverage Status

Coverage remained the same at 78.727% when pulling ab096e9 on mureinik:FILEUPLOAD-302-jupiter-migration into 273cb11 on apache:master.

@kinow
kinow approved these changes Oct 7, 2019
Copy link
Member

kinow left a comment

Looks good to me. Thanks for simplifying the PR, it was extremely easy to review as you minimized the changes to only what was necessary.

@mureinik mureinik force-pushed the mureinik:FILEUPLOAD-302-jupiter-migration branch from ebe28d4 to a630f05 Oct 7, 2019
mureinik added 2 commits Oct 7, 2019
junit.framework.TestCase was introduced in JUnit 3, and while it
isn't technically deprecated, it's outdated, and should not be used
in JUnit 4.

This patch removes its usage in the project, and replaces it with
static imports from org.junit.Assert, as per JUnit 4's best
practices.

In addition, `@Test` annotations were added to the test methods, so
they are recognized as tests by JUnit 4.
This patch upgrades the project's testing framework from JUnit 4.12
to the modern JUnit Jupiter 5.5.2.

Since JUnit 5 Jupiter is not backwards compatible to JUnit 4.x (or
even JUnit Vintage), this patch is a bit large, even though a lot of
the changes are merely cosmetic (such as changing the argument order,
see details below). In order to make the reviewer's task as easy as
possible, this PR does not presume to use JUnit Jupiter's best
practices and all its new functionality, but only to migrate the
existing tests with as little change as possible. Following patches
may want to improve the tests by using some of JUnit Jupiter's new
features.

This patch includes the following changes:

1. Maven dependency changes:
 a. junit:junit was replaced with org.junit.jupiter:junit-jupiter.

2. Annotations:
 a. org.junit.jupiter.api.Test was used as a drop in replacement for
    org.juit.Test without arguments. See 3.ii. for handling of @test
    annotations with an "expected" argument.
 b. org.junit.jupiter.api.BeforeEach was used as an drop in
    replacement for org.junit.Before.
 c. org.junit.jupiter.api.AfterEach was used as an drop in
    replacement for org.junit.After.

3. Assertions:
 a. org.junit.jupiter.api.Assertions' methods were used as drop in
    replacements for org.junit.Assert's methods with the same name in
    the simple case of an assertion without a message. In the case of
    an assertion with a message, org.junit.jupiter.api.Assertions'
    methods were used, but the argument order was changed - Assert's
    methods take the message as the first argument, while Assertions'
    methods take the message as the last argument.
 b. org.junit.jupiter.api.Assertions#assertThrows was used to assert
    that a specific exception was throws instead of an org.junit.Test
    annotation with an expected argument. As a side bonus, this
    change makes the tests slightly stricter, as now they can assert
    the exception was thrown from a specific line and prevent false
    positives where the test's "set-up" code accidentally threw that
    exception.

4. Parameterized tests:
 a. FileUploadTest was rewritten with @ParameterizedTests and
    @MethodSource in order to gain an equivalent functionality of
    JUnit 4's Parameterized runner.
@mureinik mureinik force-pushed the mureinik:FILEUPLOAD-302-jupiter-migration branch from a630f05 to ab096e9 Oct 7, 2019
@mureinik

This comment has been minimized.

Copy link
Contributor Author

mureinik commented Oct 7, 2019

@kinow the failure in @coveralls above unveiled a mistake where I forgot to add @Test annotations after removing the TestCase dependency in StreamingTest (point 2.ii in the PR message).

It's fixed now, and the coverage is OK again.
(so pending a second review or merge based on the previous approval)

@garydgregory garydgregory merged commit 2317552 into apache:master Oct 7, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 78.727%
Details
@garydgregory

This comment has been minimized.

Copy link
Member

garydgregory commented Oct 7, 2019

@mureinik Thank you for the PR. :-)
@kinow Thank you for the review. :-)

@mureinik mureinik deleted the mureinik:FILEUPLOAD-302-jupiter-migration branch Oct 7, 2019
@jochenw

This comment has been minimized.

Copy link
Contributor

jochenw commented Nov 26, 2019

What benefit does Jupiter provide, apart from "We're using the latest, and greatest."?

Besides, doesn't Jupiter have a compatibility layer, tha would allow to update without changing the test classes?

(I think, it's called "Vintage".)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.