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

JUnit v4 -> v5 Migration #524

Merged
merged 7 commits into from Oct 6, 2019

Conversation

@ham1
Copy link
Contributor

ham1 commented Oct 3, 2019

  • Ignored -> Disabled.
  • @Test(expected ...) -> Assertions.assertThrows(...)
  • A few Assert -> Assertions
    • These will be completed in another PR
  • Removed error collector.
  • Some whitespace.
@vlsi

This comment has been minimized.

Copy link
Collaborator

vlsi commented Oct 3, 2019

What is your suggestion to invite new contributions to use JUnit 5 rather than 4?

@ham1

This comment has been minimized.

Copy link
Contributor Author

ham1 commented Oct 3, 2019

I prefer Spock, because I find it often more expressive and convenient (e.g. accessing private methods).
But if you had to use JUnit then it's good for the lambda support and generally more sensible: https://developer.ibm.com/dwblog/2017/top-five-reasons-to-use-junit-5-java/

@vlsi

This comment has been minimized.

Copy link
Collaborator

vlsi commented Oct 3, 2019

accessing private methods

which is a code smell anyway, isn't it?

@vlsi

This comment has been minimized.

Copy link
Collaborator

vlsi commented Oct 3, 2019

But if you had to use JUnit then it's good for the lambda support and generally more sensible:

It looks like my intention was not well understood.

The current build script allows writing new tests with JUnit4 style.
I think it would be great if it either completely denied JUnit4 (with clear message like "plz use JUnit5") or suggested the refactoring (e.g. a set of spotless rules over test sources that convert junit annotations and so on).

@ham1

This comment has been minimized.

Copy link
Contributor Author

ham1 commented Oct 3, 2019

accessing private methods

which is a code smell anyway, isn't it?

Correct; most of the code here hasn't been TDD'd and it feels too much effort for such large restructuring of code so the smells go away?
I find it useful for enabling testing for some of the code we already have.
I think it's better than adding @deprecated to a method just for testing.

@ham1

This comment has been minimized.

Copy link
Contributor Author

ham1 commented Oct 3, 2019

But if you had to use JUnit then it's good for the lambda support and generally more sensible:

It looks like my intention was not well understood.

The current build script allows writing new tests with JUnit4 style.
I think it would be great if it either completely denied JUnit4 (with clear message like "plz use JUnit5") or suggested the refactoring (e.g. a set of spotless rules over test sources that convert junit annotations and so on).

How about a check somewhere, checkstyle?, for import org.junit.Test/Before/After/Assert/Assume/Rule

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 3, 2019

Codecov Report

Merging #524 into master will decrease coverage by 0.01%.
The diff coverage is 11.53%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #524      +/-   ##
============================================
- Coverage     56.16%   56.15%   -0.02%     
+ Complexity    10017    10014       -3     
============================================
  Files          1024     1024              
  Lines         62913    62918       +5     
  Branches       7064     7064              
============================================
- Hits          35337    35333       -4     
- Misses        25101    25108       +7     
- Partials       2475     2477       +2
Impacted Files Coverage Δ Complexity Δ
...otocol/http/gui/action/ParseCurlCommandAction.java 54.12% <0%> (-0.23%) 79 <0> (ø)
...ter/protocol/http/sampler/ResourcesDownloader.java 50.74% <13.04%> (-0.82%) 8 <0> (ø)
...in/java/org/apache/jmeter/functions/BeanShell.java 71.73% <0%> (-4.35%) 9% <0%> (ø)
...n/java/org/apache/jmeter/reporters/Summariser.java 85.49% <0%> (-0.77%) 18% <0%> (-1%)
...ache/jmeter/protocol/http/sampler/HTTPHC4Impl.java 64.59% <0%> (-0.26%) 93% <0%> (-2%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e2dda1...6fbec99. Read the comment docs.

@vlsi

This comment has been minimized.

Copy link
Collaborator

vlsi commented Oct 3, 2019

I find it useful for enabling testing for some of the code we already have.

Tests that peek onto the implementation details might become hard to maintain.

I think it's better than adding @deprecated to a method just for testing.

I'm about to add https://github.com/apiguardian-team/apiguardian so we could do things like @API(status = INTERNAL, since = "5.2.0")

@vlsi

This comment has been minimized.

Copy link
Collaborator

vlsi commented Oct 3, 2019

How about a check somewhere, checkstyle?,

checkstyle is bad because its messages are not actionable.

I suggest two options:
a) Spotless. It is nice for cases when it can auto-correct (e.g. suggest a proper test package, and it might be even enough).
b) https://github.com/policeman-tools/forbidden-apis

@ham1

This comment has been minimized.

Copy link
Contributor Author

ham1 commented Oct 3, 2019

How about a check somewhere, checkstyle?,

checkstyle is bad because its messages are not actionable.

True, however based on my experience so far, you can only (easily) auto-correct Test/Before/After etc. Assertions and Rules etc. require more complexity.

I suggest two options:
a) Spotless. It is nice for cases when it can auto-correct (e.g. suggest a proper test package, and it might be even enough).
b) https://github.com/policeman-tools/forbidden-apis

I'm not sure I like the idea of yet more libraries and code, I think I prefer adding the simple cases to Spotless so it can correct the majority but also some documentation somewhere obvious...

@vlsi

This comment has been minimized.

Copy link
Collaborator

vlsi commented Oct 4, 2019

you can only (easily) auto-correct Test/Before/After etc.

This PR contains such cases, when simple package update was enough.

also some documentation somewhere obvious

I don't suggest to implement a tool that automatically converts any JUnit4 code to JUnit5.
The idea is the tool should automatically convert known items, and it should try to print meaningful messages where it can't. Of course, certain cases will be out of scope, but I think it would help people to get familiar with the style.

@vlsi

This comment has been minimized.

Copy link
Collaborator

vlsi commented Oct 4, 2019

It looks like this PR reduces the code coverage. @ham1 , have you analyzed why is that?
For instance, it says src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/sampler/ResourcesDownloader.java is no longer covered.

@ham1 ham1 force-pushed the ham1:junit-v4-to-v5 branch from 8e9a8e7 to 0a47f8e Oct 4, 2019
@ham1

This comment has been minimized.

Copy link
Contributor Author

ham1 commented Oct 4, 2019

It looks like this PR reduces the code coverage. @ham1 , have you analyzed why is that?
For instance, it says src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/sampler/ResourcesDownloader.java is no longer covered.

I had a quick look but I don't understand because there isn't and hasn't been a unit test for it before.

@pmouawad

This comment has been minimized.

Copy link
Contributor

pmouawad commented Oct 4, 2019

Wasn't it covered through integration tests ?

@ham1 ham1 force-pushed the ham1:junit-v4-to-v5 branch from 0a47f8e to d4e4030 Oct 4, 2019
@vlsi

This comment has been minimized.

Copy link
Collaborator

vlsi commented Oct 4, 2019

Wasn't it covered through integration tests ?

@pmouawad , I've added throw exception, and it shows that ResourceDownloader was covered in org.apache.jmeter.protocol.http.sampler.ParallelResourcesAndIpSource:
https://travis-ci.org/vlsi/jmeter/jobs/593595044#L1346-L1373

@ham1 , could you try adding the same if (true) { throw ...} to your branch so we see how it goes?

@ham1

This comment has been minimized.

Copy link
Contributor Author

ham1 commented Oct 4, 2019

Ah, now I know why I disabled the test in ParallelResourcesAndIpSource because org.apache.jmeter.protocol.http.sampler.ParallelResourcesAndIpSource > test(String, WireMockServer, JMeterVariables)[1] FAILED always fails on my machine when trying to connect to wiremock.

Too many threads? Even running ./gradlew :src:protocol:http:test fails all the time.

 org.opentest4j.AssertionFailedError: expected: <url: http://wiremock/index.html
    response: OK
    data.size: 85
    data: <html><body><img src='image1.png'><img src='image2.png'><img src='image3.png'></body>
    - url: http://wiremock/image1.png
      response: OK
      data.size: 8
      data: content1
    - url: http://wiremock/image2.png
      response: OK
      data.size: 8
      data: content2
    - url: http://wiremock/image3.png
      response: OK
      data.size: 8
      data: content3
    - url: http://wiremock/index.html
      response: OK
      data.size: 85
      data: <html><body><img src='image1.png'><img src='image2.png'><img src='image3.png'></body>
    > but was: <url: http://wiremock/index.html
    response: Non HTTP response message: Connect to localhost:33099 [localhost/127.0.0.1] failed: Network is unreachable (connect failed)
    data.size: 13705
    data: org.apache.http.conn.HttpHostConnectException: Connect to localhost:33099 [localhost/127.0.0.1] failed: Network is unreachable (connect failed)
        at org.apache.http.impl.conn.DefaultHttpClientConnectionOperator.connect(DefaultHttpClientConnectionOperator.java:156)
        at org.apache.jmeter.protocol.http.sampler.HTTPHC4Impl$JMeterDefaultHttpClientConnectionOperator.connect(HTTPHC4Impl.java:326)
        at org.apache.http.impl.conn.PoolingHttpClientConnectionManager.connect(PoolingHttpClientConnectionManager.java:374)
        at org.apache.http.impl.execchain.MainClientExec.establishRoute(MainClientExec.java:393)
        at org.apache.http.impl.execchain.MainClientExec.execute(MainClientExec.java:236)
        at org.apache.http.impl.execchain.ProtocolExec.execute(ProtocolExec.java:186)
        at org.apache.http.impl.execchain.RetryExec.execute(RetryExec.java:89)
        at org.apache.http.impl.execchain.RedirectExec.execute(RedirectExec.java:110)
        at org.apache.http.impl.client.InternalHttpClient.doExecute(InternalHttpClient.java:185)
        at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:83)
        at org.apache.jmeter.protocol.http.sampler.HTTPHC4Impl.executeRequest(HTTPHC4Impl.java:850)
        at org.apache.jmeter.protocol.http.sampler.HTTPHC4Impl.sample(HTTPHC4Impl.java:561)
        at org.apache.jmeter.protocol.http.sampler.HTTPSamplerProxy.sample(HTTPSamplerProxy.java:67)
        at org.apache.jmeter.protocol.http.sampler.HTTPSamplerBase.sample(HTTPSamplerBase.java:1282)
        at org.apache.jmeter.protocol.http.sampler.ParallelResourcesAndIpSource.test(ParallelResourcesAndIpSource.java:107)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
@vlsi

This comment has been minimized.

Copy link
Collaborator

vlsi commented Oct 5, 2019

@ham1 , can you please rebase and verify if ParallelResourcesAndIpSource works now?

@ham1 ham1 force-pushed the ham1:junit-v4-to-v5 branch from a2b52dc to 6fbec99 Oct 5, 2019
@ham1

This comment has been minimized.

Copy link
Contributor Author

ham1 commented Oct 5, 2019

Works for me now 😄

@ham1 ham1 force-pushed the ham1:junit-v4-to-v5 branch from 6fbec99 to 588a074 Oct 5, 2019
@pmouawad pmouawad merged commit e68d320 into apache:master Oct 6, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@BeforeAll
public static void setup(@TempDir Path keystoreDir) throws IOException {
@BeforeEach
public void setup(@TempDir Path keystoreDir) throws IOException {
keystore = keystoreDir.resolve("dummy-keystore.jks").toFile();
password = JOrphanUtils.generateRandomAlphanumericPassword(32);

This comment has been minimized.

Copy link
@vlsi

vlsi Oct 7, 2019

Collaborator

@ham1 , why is this change?
you are initializing static fields in individual tests.

Please, please, refrain from pushing unrelated changes under a single commit.
Here you replace "junit5->juni5" code under "junit4->junit5" name. That is super-confuse those who review the change.

/cc @pmouawad

This comment has been minimized.

Copy link
@ham1

ham1 Oct 7, 2019

Author Contributor

I don't remember this change, given its single thread and still has static fields, it doesn't look a required change.

This comment has been minimized.

Copy link
@ham1

ham1 Oct 7, 2019

Author Contributor

What might have happened is some global find/replace broke this but I fixed it incorrectly and didn't notice it.

FSchumacher added a commit to FSchumacher/jmeter that referenced this pull request Oct 9, 2019
* org.junit.Test/Before/After -> ...jupiter.api.Test/BeforeEach/AfterEach
Ignored -> Disabled.
@test(expected ...) -> Assertions.assertThrows(...)
A few Assert -> Assertions.
Removed error collector.
Some whitespace.

* Added method for repeated code

* TestStringtoFile: Migrated to JUnit5

* BasicCurlParserTest: Migrated to JUnit 5

* ParseCurlCommandActionTest: JUnit 5 + tidy ParseCurlCommandAction

* ResourcesDownloader: Formatting and guard clause

* ParseCurlCommandActionTest: Use TmpDir only where required.
Also some formatting and simplification of throws.
@ham1 ham1 deleted the ham1:junit-v4-to-v5 branch Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.