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

feat: value class support #633

Merged
merged 9 commits into from Jun 17, 2021
Merged

feat: value class support #633

merged 9 commits into from Jun 17, 2021

Conversation

qoomon
Copy link
Contributor

@qoomon qoomon commented May 27, 2021

No description provided.

@qoomon qoomon mentioned this pull request May 27, 2021
3 tasks
@Raibaz
Copy link
Collaborator

Raibaz commented Jun 5, 2021

Looks like all the builds running on 1.3.72 and 1.4.30 are failing because maxOrNull has been introduced in 1.5.0.

I'd like to maintain backwards compatibility with both of them for a little while now - and i think reverting the apiVersion/languageVersion changes should fix this.

I know it gives a warning in 1.5 mentioning that apiVersion 1.3 is going to be removed, but I'd rather bear with the warning for now instead of breaking backwards compatibility.

What do you think?

@qoomon
Copy link
Contributor Author

qoomon commented Jun 6, 2021

I'll try this. However how to run test with api version 1.3 and the new value class only available in api version 1.5?
I could use inline class however this is a kinda bad test than. What do you think?

@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2021

Codecov Report

Merging #633 (fe07e44) into master (bd0a0c8) will decrease coverage by 0.48%.
The diff coverage is 4.76%.

❗ Current head fe07e44 differs from pull request most recent head c4d1bd7. Consider uploading reports for the commit c4d1bd7 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##             master     #633      +/-   ##
============================================
- Coverage     75.82%   75.33%   -0.49%     
+ Complexity      819      817       -2     
============================================
  Files           100      101       +1     
  Lines          2825     2846      +21     
  Branches        445      448       +3     
============================================
+ Hits           2142     2144       +2     
- Misses          503      522      +19     
  Partials        180      180              
Impacted Files Coverage Δ
...lin/io/mockk/proxy/jvm/advice/ValueClassSupport.kt 0.00% <0.00%> (ø)
...n/io/mockk/impl/recording/states/RecordingState.kt 91.39% <ø> (ø)
...kotlin/io/mockk/impl/verify/VerificationHelpers.kt 80.00% <ø> (ø)
...in/kotlin/io/mockk/proxy/jvm/advice/Interceptor.kt 85.71% <100.00%> (-14.29%) ⬇️
...c/main/kotlin/io/mockk/impl/stub/StubRepository.kt 66.66% <0.00%> (-8.34%) ⬇️
...oxy/jvm/transformation/FixParameterNamesVisitor.kt 77.77% <0.00%> (+3.70%) ⬆️
...lin/io/mockk/impl/platform/JvmWeakConcurrentMap.kt 56.00% <0.00%> (+4.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd0a0c8...c4d1bd7. Read the comment docs.

@qoomon
Copy link
Contributor Author

qoomon commented Jun 6, 2021

It's working. I've revered maxOrNull() to max() and suppress the compile errors by using @Suppress("DEPRECATION_ERROR")

@peterfigure
Copy link

Thanks for this PR, I've been trying it out locally and it's working well, really hope this gets merged soon! 🙏

@peterfigure
Copy link

Any way to move this PR forward? Very hard to release projects based on Kotlin 1.5 without this 😅

@Raibaz
Copy link
Collaborator

Raibaz commented Jun 15, 2021

It's currently still failing because the mockk-android module is lacking the ValueClassSupport class, which in turn is making a lot of tests fail.

@qoomon, do you think we could move it to mockk-common, so that it is also visible to the android module?

@qoomon
Copy link
Contributor Author

qoomon commented Jun 15, 2021

@Raibaz it is not that easy cause ValueClassSupport depends on kotlin.reflect.jvm.isAccessible and java.declaredMethods (this is just needed for kotlin <1.5 polyfill isValue method)

@qoomon
Copy link
Contributor Author

qoomon commented Jun 15, 2021

@Raibaz why is mockk-agent-jvm excluded within mokk-android build.gradle

   api(mockKProject, {
        exclude group: 'io.mockk', module: 'mockk-agent-jvm'
    })

@Raibaz
Copy link
Collaborator

Raibaz commented Jun 15, 2021

It's excluded because android has its own agent.

As a possible workaround for this, how about providing a dummy implementation of ValueClassSupport for mockk-android, that basically behaves as the polyfilled version for kotlin <1.5?

This will not provide support for value classes in Android, but it would still allow us to merge this.

What do you think?

@qoomon
Copy link
Contributor Author

qoomon commented Jun 16, 2021

Sounds reasonable. Is there an easy way to test if it works without the need of a full android environment?

@qoomon
Copy link
Contributor Author

qoomon commented Jun 16, 2021

I did some adjustments so it should work now, however I'm not sure if the changes within mockk-agent-android are working because I don't have the right environment

@Raibaz
Copy link
Collaborator

Raibaz commented Jun 16, 2021

Thanks a lot for taking care of this!

Unfortunately, Github requires manual approval for first-time contributors, so I have to manually trigger the builds which are, effectively, the fastest way to make sure everything works on Android.

Sorry about this :(

@qoomon
Copy link
Contributor Author

qoomon commented Jun 16, 2021

it seems all test are passing. :-D

@Raibaz
Copy link
Collaborator

Raibaz commented Jun 17, 2021

Thanks a lot again!

@pawelsmagala
Copy link

@Raibaz when will you make new mockk release with this PR included?

@Raibaz
Copy link
Collaborator

Raibaz commented Jun 23, 2021

I have a couple of PRs pending that I'd like to merge to release them all together, hopefully by the end of this week or in the next week.

@peterfigure
Copy link

@Raibaz any news on the release including this PR?

@antondudakov
Copy link

ship it 🚀

@hrach
Copy link
Contributor

hrach commented Jun 30, 2021

Guys, if you wanna help you may use prebuilts via jitpack.io. I am doing so and just discovered an issue, see latest PR. These comment won't speed this up.

@schernetsky
Copy link

schernetsky commented Jul 2, 2021

With this change, I'm seeing unboxing enter an infinite loop (inside every block) when mocking methods returning nullable value classes.

On Kotlin 1.5.10 with MockK 1.12.0

Opened issue: #656

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

10 participants