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.

Sign up for GitHub

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

Jump to bottom

Properly handle sealed classes with Kotlin 1.7 and JDK 17 #916

Merged
merged 1 commit into from Sep 24, 2022
Merged

Properly handle sealed classes with Kotlin 1.7 and JDK 17 #916

merged 1 commit into from Sep 24, 2022

Conversation

stuebingerb
Copy link
Contributor

@stuebingerb stuebingerb commented Sep 6, 2022 •
edited

Attempt to fix #832 by supporting sealed classes based on previous work done by @aSemy.

@stuebingerb stuebingerb mentioned this pull request Sep 6, 2022
Closed
@stuebingerb stuebingerb force-pushed the fix/fix-sealed-classes branch 3 times, most recently from 3c0aeec to 80c7ea3 Compare September 6, 2022 10:14
@stuebingerb stuebingerb marked this pull request as ready for review September 6, 2022 11:05
@stuebingerb stuebingerb marked this pull request as draft September 6, 2022 14:51
@stuebingerb

This comment was marked as resolved.

Sign in to view
@stuebingerb

This comment was marked as outdated.

Sign in to view
@aSemy
Copy link
Contributor

aSemy commented Sep 6, 2022

Would you be able to add a test using the example that failed? (A sealed class with an abstract child?)

@stuebingerb
Copy link
Contributor Author

stuebingerb commented Sep 6, 2022 •
edited

Would you be able to add a test using the example that failed? (A sealed class with an abstract child?)

Tried, but that test also succeeded without my modifications. I'm still missing something. Not sure if it's actually related to the abstract class.

@stuebingerb

This comment was marked as outdated.

Sign in to view
@aSemy
Copy link
Contributor

aSemy commented Sep 6, 2022

Yes the stacktrace helps, thanks, although I can't figure out exactly what's wrong. At least I think it shows that we're on the right path! I don't see any reference to ObjenesisInstantiator.kt. The last MockK code called is in io.mockk.proxy.jvm.transformation.SubclassInstrumentation, so something in there (or earlier) isn't handling sealed subclasses properly. I suspect a similar fix will be needed.

@stuebingerb

This comment was marked as outdated.

Sign in to view
@stuebingerb
Copy link
Contributor Author

stuebingerb commented Sep 7, 2022 •
edited

Sorry for commit spam. The current implementation here now seems to fix both, mockk's tests and our own. Please kindly review to spot anything I might have missed.

(I've also hidden a few of my now outdated comments)

@stuebingerb stuebingerb marked this pull request as ready for review September 7, 2022 09:48
@stuebingerb
Copy link
Contributor Author

stuebingerb commented Sep 8, 2022 •
edited

So... I tried to add a new test case based on our code and I'm claiming that something in the CI test matrix is currently broken. Have a look at stuebingerb#10 where the SealedClassTest and SealedInterfaceTest are both running successfully without any adjustments to the implementation.

@aSemy Any idea what might be wrong there?

@aSemy
Copy link
Contributor

aSemy commented Sep 8, 2022

That is indeed weird! Good catch. I think the new Gradle GHA is re-using the cache for different JDK versions. I see this message in the logs: Task :modules:mockk:test SKIPPED

@aSemy aSemy mentioned this pull request Sep 8, 2022
Open
@stuebingerb
Copy link
Contributor Author

@Raibaz @aSemy What is needed to get this merged?

@aSemy
Copy link
Contributor

aSemy commented Sep 23, 2022

I thought that because there was an issue with the Gradle tests not failing #916 (comment) that it wasn't clear if this PR would work as expected.

I don't have any suggestions on how to move forward...

@stuebingerb
Copy link
Contributor Author

stuebingerb commented Sep 23, 2022 •
edited

I guess someone will have to fix the CI. But I currently cannot. Until then I'm claiming that this PR will at least improve things and seems to fix the issues for us.

@aSemy
Copy link
Contributor

aSemy commented Sep 24, 2022

Let's do it then 👍

@aSemy aSemy mentioned this pull request Sep 24, 2022
Closed
@Raibaz
Copy link
Collaborator

Raibaz commented Sep 24, 2022

Ok makes sense, sorry I lost track of what was going on here :)

@Raibaz Raibaz merged commit 159a50a into mockk:master Sep 24, 2022
@stuebingerb stuebingerb deleted the fix/fix-sealed-classes branch September 25, 2022 13:03
@Rattenkrieg Rattenkrieg mentioned this pull request Sep 26, 2022
Closed
@SimonMarquis SimonMarquis mentioned this pull request Oct 7, 2022
Closed
@DHosseiny
Copy link

DHosseiny commented Nov 2, 2022 •
edited

This change(I guess) breaks mocking a sealed class with kotlin 1.6.21.
In my case the sealed class has abstract class/data class/object childs.

@vudzkostek vudzkostek mentioned this pull request Jan 2, 2023
Open
@vudzkostek vudzkostek mentioned this pull request Apr 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aSemy aSemy aSemy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

java.lang.InstantiationError when upgrading to Kotlin 1.7
4 participants
@stuebingerb @aSemy @Raibaz @DHosseiny