Skip to content

Conversation

@lukas-krecan
Copy link
Contributor

No description provided.

@lukas-krecan
Copy link
Contributor Author

It's not compatible with older Mockito versions so it should be a major version release.

@emartynov
Copy link

Is it for #474 ?

@lukas-krecan
Copy link
Contributor Author

Is it for #474 ?

Yes, and #478

@lukas-krecan
Copy link
Contributor Author

I just bumped Java version in CI as Mockito 5 requires Java 11

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

I expected more of the tests to fail on the varargs, but J barely see any breakage. Is this because Kotlin doesn't really use varargs or doesn't run into the problems Java has with it?

@TimvdLippe
Copy link
Contributor

@lukas-krecan Can you please answer the above question? Then this is ready to merge

@lukas-krecan
Copy link
Contributor Author

Sorry, I do not understand. Which tests do you mean?

@TimvdLippe
Copy link
Contributor

Mockito 5 made breaking changes to varargs matchers. I expected that to have consequences for Kotlin as well. However, I see little to no changes in the regression test suite of mockito-kotlin in this PR. Does that mean that these breaking changes were not relevant for Kotlin folks, or that we are missing test suite coverage?

@lukas-krecan
Copy link
Contributor Author

Does that mean that these breaking changes were not relevant for Kotlin folks, or that we are missing test suite coverage?

I guess both. The only impacted method here is anyVararg, I had to add a tests as I did not trust the current coverage. Moreover, in Kotlin you can't just use an array in vararg, you have to use the method(*array) syntax. Maybe it helped.

But I am a first-time contributor to this library, so taky me with a huge grain of salt.

@lukas-krecan
Copy link
Contributor Author

Just for completenes, changing the signature to fun <T : Any> anyVararg(clazz: KClass<T>): Array<T> and using it like this method(*anyVararg()) might be cleaner, but would complicate the migration path without much additional value.

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

Sounds good to me, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants