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

Rule compatible with old junit #102

Closed

Conversation

jerzykrlk
Copy link
Contributor

  • Mockito Rule based on MethodRule:
    • compatible with JUnit 4.7+, instead of 4.9+,
    • no need to initialize with new MockitoJUnitRule(this) - just new MockitoJUnitRule(),
  • minor: ugly fix for building Gradle in Windows (Mockito won't build on a Windows machine #97),

@mockitoguy
Copy link
Member

Thanks! Are those changes backwards compatible?

@jerzykrlk
Copy link
Contributor Author

Not with the previous version of rule. The constructor has changed.
9 paź 2014 22:40 "Szczepan Faber" notifications@github.com napisał(a):

Thanks! Are those changes backwards compatible?


Reply to this email directly or view it on GitHub
#102 (comment).

@mockitoguy
Copy link
Member

Oh I see.

We can only include backwards incompatible changes in major version changes
(we're more-less following http://semver.org/). I don't think we're ready
to do Mockito 2.0, yet :)

On Fri, Oct 10, 2014 at 6:26 AM, jerzykrlk notifications@github.com wrote:

Not with the previous version of rule. The constructor has changed.
9 paź 2014 22:40 "Szczepan Faber" notifications@github.com napisał(a):

Thanks! Are those changes backwards compatible?


Reply to this email directly or view it on GitHub
#102 (comment).


Reply to this email directly or view it on GitHub
#102 (comment).

Szczepan Faber
Core dev@gradle; Founder@mockito

@jerzykrlk
Copy link
Contributor Author

Would adding a second dummy constructor, like @Deprecated MockitoJUnitRule(Object unusedTarget) count as backward compatibility? :)
Unfortunately, the implemented interface changed (TestRule -> MethodRule), too - what do you think?

@mockitoguy
Copy link
Member

Would adding a second dummy constructor, like @Deprecated MockitoJUnitRule(Object unusedTarget) count as backward compatibility? :)

Yes :)

Unfortunately, the implemented interface changed (TestRule -> MethodRule), too - what do you think?

I'd say it's ok to change it. It's an incompatible change but feels OK.

It's interesting if we could have a method like: MockitoJUnit.rule() that
creates the rule and exposes only an interface.

On Fri, Oct 10, 2014 at 9:01 PM, jerzykrlk notifications@github.com wrote:

Would adding a second dummy constructor, like @deprecated
MockitoJUnitRule(Object unusedTarget) count as backward compatibility? :)
Unfortunately, the implemented interface changed (TestRule -> MethodRule),
too - what do you think?

Reply to this email directly or view it on GitHub
#102 (comment).

Szczepan Faber
Core dev@gradle; Founder@mockito

@jerzykrlk jerzykrlk force-pushed the rule-compatible-with-old-junit branch 2 times, most recently from 0e015a5 to 06b4301 Compare October 11, 2014 12:14
@jerzykrlk
Copy link
Contributor Author

Here we go.
For now, MockitoJUnitRule.rule() returns MockitoJUnitRule. I took a look at org.junit.rules.ExpectedException first, just in case.

@mockitoguy
Copy link
Member

This looks good, thanks!

API I'm thinking about is (does not have to be a part of this RB, just sharing the vision):

  • We add new class "MockitoJUnit" that will contain all junit-integration stuff.
  • New MockitoJUnit.rule() method that returns an interface in the signature (e.g. not a concrete class). This interface needs to inherit from JUnit hierarchy, for example MethodRule.
  • deprecate entire MockitoJUnitRule class on the grounds of concrete class leaking to the api

@bric3, wdyt?

@jerzykrlk
Copy link
Contributor Author

I think the conversation stopped for a while.

I can add MockitoJUnit.rule() soon, if we all agree.

After deprecating the entire MockitoJUnitRule class, would you create a new class to be returned from rule(), or still return MockitoJUnitRule?

@mockitoguy
Copy link
Member

Don't worry, it's on my radar. Just been busy lately :)

On Tue, Oct 21, 2014 at 7:56 AM, jerzykrlk notifications@github.com wrote:

I think the conversation stopped for a while.

I can add MockitoJUnit.rule() soon, if we all agree.

After deprecating the entire MockitoJUnitRule class, would you create a
new class to be returned from rule(), or still return MockitoJUnitRule?

Reply to this email directly or view it on GitHub
#102 (comment).

Szczepan Faber
Core dev@gradle; Founder@mockito

@bric3
Copy link
Contributor

bric3 commented Oct 27, 2014

  • We add new class MockitoJUnit that will contain all junit-integration stuff.

At some point it could make sense. Note we still have not released the TestNG integration.

  • New MockitoJUnit.rule() method that returns an interface in the signature (e.g. not a concrete class). This interface needs to inherit from JUnit hierarchy, for example MethodRule.

Agreed. For public API this is definitely recommended.

  • deprecate entire MockitoJUnitRule class on the grounds of concrete class leaking to the api

OK

After deprecating the entire MockitoJUnitRule class, would you create a new class to be returned from rule(), or still return MockitoJUnitRule ?

That may not really matter as the API of rule exposes only an interface. Maybe MockitoJunitRule can be split in two, with the main code moved to internal, and the actual type would just extends the internal class (and still be deprecated)

configure([jar, allJar]) { task ->
task.rootSpec.exclude "MANIFEST.MF" //hack to avoid problems with bnd
doLast {
project.exec {
commandLine 'ant', '-f', 'build-ant.xml', "osgify.$task.baseName", "-Dversion=$project.version"
commandLine antCommand, '-f', 'build-ant.xml', "osgify.$task.baseName", "-Dversion=$project.version"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you split this change in a separate commit. (Even in the same PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see here: #115

@jerzykrlk jerzykrlk force-pushed the rule-compatible-with-old-junit branch from 1bf195d to d13f097 Compare November 19, 2014 21:10
@mockitoguy
Copy link
Member

Attempting merge...

@mockitoguy
Copy link
Member

Merged!

@mockitoguy mockitoguy closed this Dec 15, 2014
neworld pushed a commit to neworld/mockito that referenced this pull request Aug 31, 2019
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.

None yet

3 participants