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

#478 Upgrade Mockito #482

Merged
merged 3 commits into from
Jun 1, 2023
Merged

#478 Upgrade Mockito #482

merged 3 commits into from
Jun 1, 2023

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants