Skip to content
This repository was archived by the owner on Jun 6, 2019. It is now read-only.

Update to Kotlin 1.3.11 and coroutines 1.0.1 #34

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

bradynpoulsen
Copy link

No description provided.

@zsmb13
Copy link

zsmb13 commented Oct 29, 2018

This is gonna be added to Retrofit, therefore I don't think this library will be updated.

See the PR for that here: square/retrofit#2886

@JakeWharton
Copy link
Owner

It will be updated but there's a few tests failures I saw locally. I'm assuming that's why CI failed

@bradynpoulsen
Copy link
Author

Yeah, It's from deprecations being removed. I'm updating those now

@bradynpoulsen
Copy link
Author

@JakeWharton So i migrated the CancelTest.noCancelOnError test to use
deferred.isCancelled && deferred.isCompleted as CompletableDeferred.isCompletedExceptionally was deprecated in coroutines 0.27.0, but that entire test is confusing to me.

It calls 1) Call<T>.completeExceptionally which 2) I assume invokes the callback's onFailure which 3) causes the deferred's invokeOnCompletion to cancel the call.

But that test case is checking that the Call was not cancelled. That seems like a errant test case

@bradynpoulsen bradynpoulsen force-pushed the feature/update_to_stable_kotlin_1.3_and_coroutines_1.0.0 branch from 48cba19 to d8ad9ca Compare October 29, 2018 20:53
@@ -36,7 +36,7 @@ class CancelTest {
val deferred = adapter.adapt(call)
call.completeWithException(IOException())
assertFalse(call.isCanceled)
assertTrue(deferred.isCompletedExceptionally)
assertTrue(deferred.isCancelled && deferred.isCompleted)
Copy link
Author

Choose a reason for hiding this comment

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

See Deferred in coroutines 0.27.0 for more details

@@ -36,7 +36,7 @@ class CancelTest {
val deferred = adapter.adapt(call)
call.completeWithException(IOException())
assertFalse(call.isCanceled)
Copy link
Author

Choose a reason for hiding this comment

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

The call is cancelled in coroutines 0.27.0+ because CompletedExceptionally is considered a "cancelled" state

@JakeWharton
Copy link
Owner

JakeWharton commented Oct 29, 2018

It wouldn't be a test case if it wasn't something we needed to guarantee. We need alter the implementation to allow the test case to pass. A call should not be marked as canceled when it completes exceptionally.

@bradynpoulsen bradynpoulsen force-pushed the feature/update_to_stable_kotlin_1.3_and_coroutines_1.0.0 branch from 2a080a6 to da744e7 Compare October 29, 2018 22:16
@bradynpoulsen
Copy link
Author

CancellationException is probably a good case to check for if we don't want the call to be cancelled by failure states. A snippet from the Job documentation:

Normal cancellation of a job is distinguished from its failure by the type of its cancellation exception cause.
If the cause of cancellation is [CancellationException], then the job is considered to be cancelled normally. This usually happens when [cancel] is invoked without additional parameters.
If the cause of cancellation is a different exception, then the job is considered to have failed. This usually happens when the code of the job encounters some problem and throws an exception.

@bradynpoulsen bradynpoulsen force-pushed the feature/update_to_stable_kotlin_1.3_and_coroutines_1.0.0 branch from da744e7 to 4b32e53 Compare October 29, 2018 22:26
@bradynpoulsen
Copy link
Author

@JakeWharton that CancellationException implementation seems to work with the existing test and makes sense with how coroutines are documented to behave

@nkraev
Copy link

nkraev commented Nov 22, 2018

Subscribing here to give better visibility that everything is resolved and can be merged, @JakeWharton :)
Also (if that matters), @bradynpoulsen fix for CancellationException is correct, i just recently performed same fixes for one of the libs while migrating to 1.3.0

@bradynpoulsen bradynpoulsen changed the title Update to Kotlin 1.3.0 and coroutines 1.0.0 Update to Kotlin 1.3.11 and coroutines 1.0.1 Dec 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants