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

java.time.Duration not present before SDK 26 #1843

Closed
isuPatches opened this issue Dec 15, 2019 · 15 comments · Fixed by #1845
Closed

java.time.Duration not present before SDK 26 #1843

isuPatches opened this issue Dec 15, 2019 · 15 comments · Fixed by #1845

Comments

@isuPatches
Copy link

Unable to use Mockito 3.1.7+ with Android on lower SDK devices.

Reason:

https://developer.android.com/reference/java/time/Duration <-- This was not added to Android until SDK 26.

https://developer.android.com/about/dashboards <-- here are some distribution stats. Most noticeably, this would impact testing for 70% of the market and is not ideal.

java.lang.NoClassDefFoundError: java.time.Duration
at org.mockito.Mockito.timeout(Mockito.java:2856)
@TimvdLippe
Copy link
Contributor

This is an unintended breakage. java.time.Duration was added in Java 8, which is our minimum supported version. Android is lacking behind in implementing these, which means that we have to come up with a compatibility guarantee for Android SDK versions.

@kluever I would like to revert your change for now until we can resolve this matter.

@kluever
Copy link
Contributor

kluever commented Dec 16, 2019

Womp womp...OK!

@bric3
Copy link
Contributor

bric3 commented Dec 16, 2019

https://developer.android.com/about/dashboards <-- here are some distribution stats. Most noticeably, this would impact testing for 70% of the market and is not ideal.

And this would impact backend even more. Mockito 3 is java 8 based, there's no reason to not use java 8 types.

Plus changing the API back and forth introduces loads of compatibility issues.

I'd rather revert #1845, and instead introduces compatibility verification modes for pre Java 8 users.

And I believe there's some new game changing feature coming to stock android tool chain if it's not already thetre: desugaring. In particular it could desugar APIs : https://jakewharton.com/androids-java-8-support/#desugaring-apis

@TimvdLippe
Copy link
Contributor

I'd rather revert #1845, and instead introduces compatibility verification modes for pre Java 8 users.

Yeah I would like to recommit java.time.Duration, but would like to solve the problem of compatibility first. Since it is unlikely anyone used this method in the short period (<1 week) it was unavailable, I think the overall impact has been minimal. Given that the value is minimal, but would break the majority of Android users, I think reverting is the correct choice.

We might be able to add the API in a separate class and exclude it in mockito-android? The more prominent problem is that Android has not supported Java 8 features for a long time, which is now a problem.

@bric3
Copy link
Contributor

bric3 commented Dec 18, 2019

The fact that Android is not real Java is the base of the issue, this is likely that Android API and JDK API diverge or evolve differently. Android is dragging behind every backend library due to this issue with API level. And Mockito 3 is Java 8, if some needs older version there's still Mockito 2.x, that is tailored for previous versions of the JDK.

My opinion with this is :
If application needs to stick with older versions of the API level, then they have to understand the project cannot update some libraries as well, and that's fine. Previous mockito versions work.
That's the same as server development, if I'm on Java 8 I cannot use the fancy new stuff that are available on some libraries that are tailored for JDK 12, that's a bummer but that's software life.

@margussipria
Copy link

@TimvdLippe Hello, "anyone" here and I managed to use it. I use compiler flag that treats deprecation warnings as errors and resolve them immediately and not let code rot years until upgrading unsupported libraries becomes yearlong project of its own.

For now I can make revert to over own commit, but it will also cause problems to others writing new test..

[error] 17 errors found
[error] (Test / compileIncremental) Compilation failed

@isuPatches
Copy link
Author

Mockito claims no breaking changes for Mockito 3...the Duration addition made this untrue. The Duration API should be separated so that Android who may not have accessibility to newer versions of Java for years can still function and update.

@margussipria
Copy link

"Mockito 3 does not introduce any breaking API changes, but now requires Java 8 over Java 6 for Mockito 2."

There wasn't any braking changes, worked fine with Java 8+ when change were combined with deprecation tags. It is point of view what is braking change, and in my point of view, first baking change happened when added API was removed.

I probably can stop my upgrades and wait 4.x. I was stuck to 1.10.19 over 2 years waiting #1074 to be resolved, now that I can use 3.2.0 (probably 3.2.2 works also) I can wait another year or more.

@isuPatches
Copy link
Author

When it breaks the majority of the market share for a platform and requires an SDK level that only supports the last 2-3 SDK levels, it most certainly is. Are you claiming in <1 week you have a full test suite that would be rendered useless by this reversion? This killed 400+ tests for me.

Just use the version before the revert :P

As mentioned above it's not going away forever, it just needs to be a separate API to be best practice for all platforms.

@bric3
Copy link
Contributor

bric3 commented Dec 18, 2019

Mockito claims no breaking changes for Mockito 3

Yes that was for users that needed to upgrade from Java 6 to Java 8. To use Mockito 3, the consuming project needs to be compiled and run with Java 8 already, that means that all APIs that were present in Mockito 2 are present in Mockito 3, but does not prevent Mockito additional API using Java 8 types to be added like Optional.

When it breaks the majority of the market share for a platform and requires an SDK level that only supports the last 2-3 SDK levels, it most certainly is.

When the project is upgraded to the latest version that is based on Java 8 minimum when the Android SDK that is not even Java 8 compatible, the responsibility is not on the library.

"Mockito 3 does not introduce any breaking API changes, but now requires Java 8 over Java 6 for Mockito 2."

That holds true as Mockito is following Java versions, not Android lifecycle. If you can compile with Java 6 it will compile the same if the consuming project uses with Java 8. Android SDK is another story.

Currently I think that old Android SDKs users should prefer Mockito 2 as the APIs of the Android SDK and Java 6 are somewhat aligned, otherwise they should assume possible breakage if using Mockito 3 which is Java 8 minimum (that means class version and runtime APIs).

@isuPatches
Copy link
Author

isuPatches commented Dec 18, 2019

Why penalize Android developers when it could be a separate API and give both parties what they want?

Again, no one is saying "take Duration and Java 8 away permanently" at the detriment of web/java developers...simply, it needs to be implemented in a compatible manner for both.

The fact is, this change is new in Mockito 3.1.7+ and the impact is more for developers updating with an existing test suite than those who are just now writing tests with this new functionality.

@TimvdLippe
Copy link
Contributor

I dont have time to provide a full comment (1AM here), but will provide one tomorrow.

For now: please accept my apology for those who used the new Duration API and were impacted by the revert. This has been an unfortunate change that broke a lot of existing Android users. Given the widespread usage of Mockito in the Android community and thus the large impact of the breaking change by relying on java.time.Duration, reverting felt like the least intrusive option. Yes, this would again break users who used the new api, but my assumption was that the amount of users that eagerly upgraded to the new api would be minimal, compared to existing users.

However, we do need to resolve this issue at large, as we should not hold back improvements on Mockito itself, because a different ecosystem is/was lacking behind. For this, I wanted to provide myself and the other core maintainers some more time to make a decision. That's why I reverted the PR in question, to allow us to make a sensible and thoughtful decision.

I will do a full write-up tomorrow and also notify the Android folks at Google of this issue. Hopefully they can figure out a solution that would not hold libraries like Mockito (but I imagine a lot of other libraries as well) back.

I apologize for any inconvenience caused. Thank you for your understanding. I will comment further tomorrow.

@bric3
Copy link
Contributor

bric3 commented Dec 19, 2019

Why penalize Android developers when it could be a separate API and give both parties what they want?

Android developers are not penalized, this is how software versionning is working. Android devs using a more recent version of the ANdroid SDK can use later versions.

You have to ask yourself if Android SDK pre 26 is not Java 8 compatible, why is the project you are working on using Java 8 libraries ?

Also, providing a separate API for Android SDK is certainly feasible, given the pace and the direction of the Andorid SDK development compared to Java, we might end up with many API classes, I'm not sure that's wise. AssertJ do that, and it's a regular pain to use as method are moved to AssertionsJava6, etc. Not to mention the code change needed when upgrading.

@JakeWharton 22h
The video of my @KotlinConf talk "What's new in Java 19: The end of Kotlin?" is now available!

Having a separate Android API is not out of question, but it has various drawbacks, including maintainability on Mockito's side as well as on the consumer side. I'll be be quite cautious on this path. I'd rather prefer saner appraoch with versionning. Or Android devs to resolve this issue, e.g. with D8 that will desugar stuff for older SDKs.

@JakeWharton 17h
Just published "D8 Library Desugaring" which covers how APIs like streams, optional, and java.time are backported in AGP 4.0.

@TimvdLippe
Copy link
Contributor

I started writing a postmortem, which you can find at https://github.com/mockito/mockito/wiki/Android-Java-8-%60java.time.Duration%60-postmortem

Since it is almost Christmas vacation, i won't be able to start working on some of the action items. In the new year, I can hopefully work on that.

@ijuma
Copy link
Contributor

ijuma commented Mar 17, 2020

@TimvdLippe Note that the market share link you used hasn't been updated for nearly 1 year:

"Data collected during a 7-day period ending on May 7, 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 a pull request may close this issue.

6 participants