retrofit2-kotlin-coroutines-adapter
retrofit2-kotlin-coroutines-adapter copied to clipboard
JakeWharton
Update to Kotlin 1.3.11 and coroutines 1.0.1
This is gonna be added to Retrofit, therefore I don't think this library will be updated.
See the PR for that here: https://github.com/square/retrofit/pull/2886
It will be updated but there's a few tests failures I saw locally. I'm assuming that's why CI failed
@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
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.
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.
@JakeWharton that CancellationException implementation seems to work with the existing test and makes sense with how coroutines are documented to behave
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
