JUnit v4 -> v5 Migration by ham1 · Pull Request #524 · apache/jmeter

ham1 · 2019-10-03T20:41:59Z

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

vlsi · 2019-10-03T20:46:27Z

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

src/core/src/test/java/org/apache/jmeter/threads/TestUnmodifiableJMeterVariables.java

ham1 · 2019-10-03T20:51:28Z

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 · 2019-10-03T20:52:04Z

accessing private methods

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

vlsi · 2019-10-03T20:54:15Z

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 · 2019-10-03T20:56:52Z

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 · 2019-10-03T20:57:56Z

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 · 2019-10-03T21:00:41Z

Codecov Report

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

@@             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 · 2019-10-03T21:07:49Z

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 · 2019-10-03T21:09:05Z

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 · 2019-10-03T21:48:07Z

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 · 2019-10-04T06:33:40Z

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 · 2019-10-04T06:35:23Z

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 · 2019-10-04T11:34:14Z

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 · 2019-10-04T11:36:29Z

Wasn't it covered through integration tests ?

vlsi · 2019-10-04T15:18:51Z

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 · 2019-10-04T16:56:26Z

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 · 2019-10-05T08:21:34Z

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

@test

Ignored -> Disabled. @test(expected ...) -> Assertions.assertThrows(...) A few Assert -> Assertions. Removed error collector. Some whitespace.

Also some formatting and simplification of throws.

ham1 · 2019-10-05T12:09:09Z

Works for me now 😄

vlsi · 2019-10-07T06:28:37Z

src/jorphan/src/test/java/org/apache/jorphan/exec/TestKeyToolUtils.java

+    @BeforeEach
+    public void setup(@TempDir Path keystoreDir) throws IOException {
        keystore = keystoreDir.resolve("dummy-keystore.jks").toFile();
        password = JOrphanUtils.generateRandomAlphanumericPassword(32);


@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

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

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

@test

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

vlsi reviewed Oct 3, 2019

View reviewed changes

src/core/src/test/java/org/apache/jmeter/threads/TestUnmodifiableJMeterVariables.java Outdated Show resolved Hide resolved

ham1 force-pushed the junit-v4-to-v5 branch from 8e9a8e7 to 0a47f8e Compare October 4, 2019 09:45

ham1 force-pushed the junit-v4-to-v5 branch from 0a47f8e to d4e4030 Compare October 4, 2019 11:45

ham1 force-pushed the junit-v4-to-v5 branch from a2b52dc to 6fbec99 Compare October 5, 2019 09:44

ham1 added 7 commits October 5, 2019 12:53

org.junit.Test/Before/After -> ...jupiter.api.Test/BeforeEach/AfterEach

f48a075

Ignored -> Disabled. @test(expected ...) -> Assertions.assertThrows(...) A few Assert -> Assertions. Removed error collector. Some whitespace.

Added method for repeated code

3e89bdd

TestStringtoFile: Migrated to JUnit5

4f185ae

BasicCurlParserTest: Migrated to JUnit 5

5159933

ParseCurlCommandActionTest: JUnit 5 + tidy ParseCurlCommandAction

6b9881f

ResourcesDownloader: Formatting and guard clause

f0502bb

ParseCurlCommandActionTest: Use TmpDir only where required.

588a074

Also some formatting and simplification of throws.

ham1 force-pushed the junit-v4-to-v5 branch from 6fbec99 to 588a074 Compare October 5, 2019 12:09

pmouawad merged commit e68d320 into apache:master Oct 6, 2019

vlsi reviewed Oct 7, 2019

View reviewed changes

ham1 deleted the junit-v4-to-v5 branch October 10, 2019 07:49

Conversation

ham1 commented Oct 3, 2019

Uh oh!

vlsi commented Oct 3, 2019

Uh oh!

Uh oh!

ham1 commented Oct 3, 2019

Uh oh!

vlsi commented Oct 3, 2019

Uh oh!

vlsi commented Oct 3, 2019

Uh oh!

ham1 commented Oct 3, 2019

Uh oh!

ham1 commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vlsi commented Oct 3, 2019

Uh oh!

vlsi commented Oct 3, 2019

Uh oh!

ham1 commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vlsi commented Oct 4, 2019

Uh oh!

vlsi commented Oct 4, 2019

Uh oh!

ham1 commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmouawad commented Oct 4, 2019

Uh oh!

vlsi commented Oct 4, 2019

Uh oh!

ham1 commented Oct 4, 2019

Uh oh!

vlsi commented Oct 5, 2019

Uh oh!

ham1 commented Oct 5, 2019

Uh oh!

vlsi Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

ham1 Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

ham1 Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ham1 commented Oct 3, 2019 •

edited

Loading

codecov-io commented Oct 3, 2019 •

edited

Loading

ham1 commented Oct 3, 2019 •

edited

Loading

ham1 commented Oct 4, 2019 •

edited

Loading