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

Regroup Junit classes in junit packages #748

Merged
merged 3 commits into from
Nov 17, 2016

Conversation

bric3
Copy link
Contributor

@bric3 bric3 commented Nov 8, 2016

This PR proposes to regroup all JUnit related classes in corrct packages. There's two changes :

  • public : Deprecates org.mockito.runners.MockitoJUnitRunner and moves logic over org.mockito.junit.runners.MockitoJUnitRunner

    Questions :

    • Should the runner be moved in the subpackage junit.runner or regroup the runner along with the rule in the junit package? (JUnit 5 will have neither of those)
    • Should I do the same for already deprecated runners (console spamming and verbose runner)
  • private : Moves JUnitHackerTool to org.mockito.internal.junit.util

@bric3
Copy link
Contributor Author

bric3 commented Nov 8, 2016

lower coverable due to the redirect classes (deprecated classes no extends the moved classes)

@codecov-io
Copy link

codecov-io commented Nov 8, 2016

Current coverage is 86.55% (diff: 70.00%)

No coverage report found for release/2.x at 205faa7.

Powered by Codecov. Last update 205faa7...3d60ea4

@ChristianSchwarz
Copy link
Contributor

Great PR!

Should the runner be moved in the subpackage junit.runner or regroup the runner along with the rule in the junit package?

I vote for a regroup into the junit package. We only have 3 classes here and I see no advantage of having an package with a single class.

@mockitoguy
Copy link
Member

@bric3, please don't merge it. The new package you suggest is much better. Thing is, it is not really adding that much value to the users, while annoying them with deprecation warnings at the same time :)

Let's keep runner where it is. It will die eventually with adoption of JUnit5.

@mockitoguy
Copy link
Member

Also consider tons of various external documentation and examples referring to runners, that would be deprecated with this change.

@bric3 bric3 added the on hold label Nov 9, 2016
@bric3
Copy link
Contributor Author

bric3 commented Nov 9, 2016

@szczepiq I'm not ready to merge it yet, however junit 4 will probably be there for a long time.
For now the switch to the new package is easy, and doesn't break anything until mockito 3. I'm really keen to see this merged. @Mock annotation was deprecated the same way before.

@ChristianSchwarz Thanks ;) OK noted I had the same feeling after the push.

@mockitoguy
Copy link
Member

Very glad to hear you want to merge code eagerly :)

I'm curious why you are so keen for this change? It introduces disturbance (deprecation of feature introduced 1 month ago, like Runner.Silent) while providing zero value to the user and negligible value to us?

Even though the packages you suggest are much better, I still strongly believe that this should not be merged.

@bric3
Copy link
Contributor Author

bric3 commented Nov 12, 2016

For one it allows to regroup junit stuff in a single package that make sense. That is always something good, if we either want to ship a separate artefact for junit, public API will already make sense. On the same topic I don't want to drop Test-NG, I feel that we can either include it in mockito-core in a testng package.

The disturbance is indeed true, but small, the types are the same, only the package changes. Note this has already happened on a new feature. MockitoAnnotations.Mock was deprecated because it moved elsewhere same as the new runner.

Even though the packages you suggest are much better, I still strongly believe that this should not be merged.

Yes they are much better. I will make the change for mockito 3, regardless of what happened in release/2.x, I'd rather prepare gently the user base on that, rather that enforce the change when 3.x is released.
This is a continuous improvement that is non breaking.

@TimvdLippe
Copy link
Contributor

Since this change is non-breaking and makes sense for later refactorings (e.g. JUnit 5 compatible will likely be in a separate project), I am 👍 to this PR

@bric3
Copy link
Contributor Author

bric3 commented Nov 17, 2016

Given the 3 👍 I'm gonna merge this pull request

Signed-off-by: Brice Dutheil <brice.dutheil@gmail.com>
This change has been made in a way that is not breaking existing code.

Signed-off-by: Brice Dutheil <brice.dutheil@gmail.com>
@bric3 bric3 merged commit 312d7e3 into mockito:release/2.x Nov 17, 2016
@bric3 bric3 deleted the tidy-up-junit-packages branch November 17, 2016 12:46
@mockitoguy
Copy link
Member

We talked about this ticket on skype with @bric3. I feel a bit let down this was merged in. However, life goes on, and we move on to other exciting improvements :)

I really like the new packages and I'm already profiting from the refactoring (#770).

@bric3
Copy link
Contributor Author

bric3 commented Nov 21, 2016

Yes I recognise this was commending, sorry about that. Though I really feel this weighs in for a better public consistent API. Imho this his had to be done at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants