Dokka and coroutine version bump #514

kggilmer · 2022-02-01T22:16:26Z

Companion PR: smithy-lang/smithy-kotlin#580

Issue #

Description of changes

Update tests to use new coroutines-test APIs. See here for more details
Update APIs using duration as integers to use stable Duration type as of Kotlin 1.6
Move some code from JVM source set to Common based on new kmp test support in 1.6
Remove test utility code no longer needed after 1.6 updated
Update mockk test dependency and fix new test regressions due to what seems like new behavior regarding mocks (partial mocks throw exceptions in some cases, have to manually provide answers in previous cases where logic correctly delegated to base implementation)

NOTE

~~9 tests~~ 2 tests has been marked with @IgnoreWindows due to test failure and regression issues associated with the coroutine-test update.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

… refactoring needs

github-actions · 2022-02-01T22:21:19Z

A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump

ianbotsf

Generally looks good (minus the new @Ignore tests obvs).

aajtodd

Looks good overall, feel free to reach out for help on the CRT issues if needed.

I guess I should have commented in smithy-kotlin PR but I was surprised the "ManualDispatch" tests (BufferedReadChannelTest.kt and BufferReadChannelByteBufferTest.kt) Just Work (TM) out of the box. Before the runTest implementation in the base class would call pauseDispatcher(), do you understand why these work out of the box with the new runTest implementation?

aajtodd · 2022-02-02T11:51:02Z

...me/http-client-engine-crt/jvm/test/aws/sdk/kotlin/runtime/http/engine/crt/AsyncStressTest.kt

@@ -40,7 +42,8 @@ class AsyncStressTest : TestWithLocalServer() {
    }

    @Test
-    fun testConcurrentRequests() = runSuspendTest {
+    @Ignore // FIXME: Test fails after kotlinx-coroutines-test 1.6.0 upgrade
+    fun testConcurrentRequests() = runTest {


If these fail with runTest I would just make it runBlocking and see if it continues to work. Can you expand on the failures that are happening though? This isn't necessarily one that I thought would require attention (although anything with CRT can be fragile due to the threading model and how it interacts with coroutines)

It's also possible that we are reliant on a particular dispatcher behavior in which case we might need to look closer at the internals.

Here is a snippet from the error when enabling the test. Happily runBlocking seems to fix it up!

[Test worker @coroutine#2] INFO aws.smithy.kotlin.runtime.httptest.TestWithLocalServer - test server listening on: localhost:46491 exception on 460: aws.sdk.kotlin.runtime.ClientException: timed out waiting for an HTTP connection to be acquired from the pool exception on 454: java.util.concurrent.CancellationException: Parent job is Cancelling ... exception on 461: aws.sdk.kotlin.runtime.ClientException: timed out waiting for an HTTP connection to be acquired from the pool exception on 462: aws.sdk.kotlin.runtime.ClientException: timed out waiting for an HTTP connection to be acquired from the pool ... [Test worker] INFO aws.smithy.kotlin.runtime.httptest.TestWithLocalServer - test server stopped timed out waiting for an HTTP connection to be acquired from the pool aws.sdk.kotlin.runtime.ClientException: timed out waiting for an HTTP connection to be acquired from the pool at app//aws.sdk.kotlin.runtime.http.engine.crt.CrtHttpEngine.roundTrip(CrtHttpEngine.kt:81)

aajtodd · 2022-02-02T12:00:08Z

services/s3/e2eTest/S3IntegrationTest.kt

    @Test
-    fun testPutObjectFromFile() = runSuspendTest {
+    @Ignore // FIXME: CRT HTTP client fails to get connection after kotlinx-coroutines-test 1.6.0 upgrade


these CRT issues lead me to believe it's probably the dispatcher, my guess is some interaction between the dispatcher and CRT trying to read the request body (or send the response body).

github-actions · 2022-02-03T00:25:37Z

A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump

github-actions · 2022-02-03T00:26:28Z

A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump

github-actions · 2022-02-03T02:19:07Z

A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump

github-actions · 2022-02-03T19:46:24Z

A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump

github-actions · 2022-02-03T21:41:21Z

A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump

github-actions · 2022-02-03T22:37:35Z

A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump

github-actions · 2022-02-03T22:50:00Z

A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump

ianbotsf · 2022-02-04T16:20:28Z

...me/http-client-engine-crt/jvm/test/aws/sdk/kotlin/runtime/http/engine/crt/AsyncStressTest.kt

@@ -73,9 +72,9 @@ class AsyncStressTest : TestWithLocalServer() {
        }
    }

+    @IgnoreWindows


Nit: Can we please add a reason to this annotation?

What's the reason for this now?

Here is the test run in which the regression occurs on Windows: https://github.com/awslabs/aws-sdk-kotlin/runs/5057351434?check_suite_focus=true

Here is the error snippet from the log:

AsyncStressTest[jvm] > testStreamNotConsumed()[jvm] STANDARD_ERROR [DefaultDispatcher-worker-1 @coroutine#2078] INFO ktor.application - Autoreload is disabled because the development mode is off. [DefaultDispatcher-worker-1 @coroutine#2078] INFO ktor.application - Responding at http://0.0.0.0:51019/ [DefaultDispatcher-worker-1 @coroutine#2078] INFO ktor.application - Application started in 0.013 seconds. [Test worker @coroutine#2079] INFO aws.smithy.kotlin.runtime.httptest.TestWithLocalServer - test server listening on: localhost:51019 AsyncStressTest[jvm] > testStreamNotConsumed()[jvm] STANDARD_OUT exception on 973: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 974: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 975: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 976: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 977: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 982: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 981: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 985: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 987: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 978: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 979: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 983: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 986: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 980: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 984: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 988: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 989: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 990: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 991: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 992: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 993: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 994: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 995: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 996: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 997: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 998: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms exception on 999: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms AsyncStressTest[jvm] > testStreamNotConsumed()[jvm] STANDARD_ERROR [Test worker] INFO aws.smithy.kotlin.runtime.httptest.TestWithLocalServer - test server stopped AsyncStressTest[jvm] > testStreamNotConsumed()[jvm] FAILED kotlinx.coroutines.TimeoutCancellationException at �:-1 Caused by: kotlinx.coroutines.TimeoutCancellationException at Timeout.kt:184

OK IDK if we can merge this without looking into this further to ensure we aren't introducing a regression on windows (i.e. is this an issue with the test surfaced by the upgrade OR a latent issue with the implementation discovered in this test by the upgrade)

Possibly related:

https://stackoverflow.com/questions/70658926/how-to-use-kotlinx-coroutines-withtimeout-in-kotlinx-coroutines-test-runtest

Add a a way to disable automatic delay skipping with runBlockingTest Kotlin/kotlinx.coroutines#1266

ianbotsf · 2022-02-04T16:21:56Z

services/build.gradle.kts

@@ -67,6 +67,9 @@ subprojects {
        kotlinOptions {
            jvmTarget = "1.8" // this is the default but it's better to be explicit (e.g. it may change in Kotlin 1.5)
            allWarningsAsErrors = false // FIXME Tons of errors occur in generated code
+            // Enable coroutine runTests in 1.6.10
+            // NOTE: may be removed after coroutines-test runTests becomes stable
+            freeCompilerArgs = freeCompilerArgs + "-opt-in=kotlin.RequiresOptIn"


Nit: freeCompilerArgs += "-opt-in=kotlin.RequiresOptIn"

I had it that way originally but the IDE warned me of the creation of a list under the hood and recommended this form instead.

ianbotsf · 2022-02-04T16:22:00Z

services/build.gradle.kts

+                kotlinOptions {
+                    // Enable coroutine runTests in 1.6.10
+                    // NOTE: may be removed after coroutines-test runTests becomes stable
+                    freeCompilerArgs = freeCompilerArgs + "-opt-in=kotlin.RequiresOptIn"


Nit: freeCompilerArgs += "-opt-in=kotlin.RequiresOptIn"

I had it that way originally but the IDE warned me of the creation of a list under the hood and recommended this form instead.

aajtodd

Other than the ignored test for windows looks good.

aajtodd · 2022-02-04T17:50:57Z

...me/http-client-engine-crt/jvm/test/aws/sdk/kotlin/runtime/http/engine/crt/AsyncStressTest.kt

@@ -73,9 +72,9 @@ class AsyncStressTest : TestWithLocalServer() {
        }
    }

+    @IgnoreWindows


What's the reason for this now?

github-actions · 2022-02-04T18:26:25Z

A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump

…on-bump

sonarcloud · 2022-02-15T18:17:35Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
12 Code Smells

No Coverage information
0.0% Duplication

github-actions · 2022-02-15T18:20:45Z

A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump

kggilmer added 10 commits January 27, 2022 09:21

WIP

9b4888a

Merge branch 'main' into refactor-dokka-1405

58af350

WIP

139bbc3

Updates for coroutine test 1.6

7782147

Updates for coroutine test 1.6

f015f11

experimental fix

17eda19

Fix tests where simple to do so, ignore others with presumably bigger…

9642911

… refactoring needs

Refactor to use Duration type over Int to model time

fc7d734

Misc fixes and disable tests broken by coroutines-test 1.6

39d9fe0

lint

68b269d

kggilmer requested a review from a team as a code owner February 1, 2022 22:16

kggilmer changed the title ~~Chore coroutine version bump~~ Dokka and coroutine version bump Feb 1, 2022

kggilmer mentioned this pull request Feb 1, 2022

Chore coroutine version bump smithy-lang/smithy-kotlin#580

Merged

ianbotsf reviewed Feb 1, 2022

View reviewed changes

aajtodd reviewed Feb 2, 2022

View reviewed changes

kggilmer added 3 commits February 2, 2022 16:20

Re-enable tests after switching to runBlocking as advised in PR

4287180

lint

6f35e85

Merge branch 'main' into chore-coroutine-version-bump

f49680d

Fix warnings found by CI

6d107e5

Fix warnings

97f13e9

Disable failing test on windows

981e85e

Fix test failures

875696a

lint fix

f58e627

ianbotsf approved these changes Feb 4, 2022

View reviewed changes

aajtodd approved these changes Feb 4, 2022

View reviewed changes

Add note regarding test regression

9ce45d6

kggilmer mentioned this pull request Feb 8, 2022

Kotlin 1.6 upgrade ~ Duration and Coroutine Test #513

Closed

aajtodd added 2 commits February 15, 2022 11:14

remove ignore windows from async stress test

b13cfaf

Merge remote-tracking branch 'origin/main' into chore-coroutine-versi…

dd1d144

…on-bump

aajtodd merged commit 430f1f8 into main Feb 15, 2022

aajtodd deleted the chore-coroutine-version-bump branch February 15, 2022 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dokka and coroutine version bump #514

Dokka and coroutine version bump #514

kggilmer commented Feb 1, 2022 •

edited

github-actions bot commented Feb 1, 2022

ianbotsf left a comment •

edited

aajtodd left a comment

aajtodd Feb 2, 2022

aajtodd Feb 2, 2022

kggilmer Feb 3, 2022

aajtodd Feb 2, 2022

github-actions bot commented Feb 3, 2022

github-actions bot commented Feb 3, 2022

github-actions bot commented Feb 3, 2022

github-actions bot commented Feb 3, 2022

github-actions bot commented Feb 3, 2022

github-actions bot commented Feb 3, 2022

github-actions bot commented Feb 3, 2022

ianbotsf Feb 4, 2022

aajtodd Feb 4, 2022

kggilmer Feb 4, 2022

aajtodd Feb 7, 2022 •

edited

kggilmer Feb 10, 2022

ianbotsf Feb 4, 2022

kggilmer Feb 4, 2022

ianbotsf Feb 4, 2022

kggilmer Feb 4, 2022

aajtodd left a comment

aajtodd Feb 4, 2022

github-actions bot commented Feb 4, 2022

sonarcloud bot commented Feb 15, 2022

github-actions bot commented Feb 15, 2022

Dokka and coroutine version bump #514

Dokka and coroutine version bump #514

Conversation

kggilmer commented Feb 1, 2022 • edited

Issue #

Description of changes

NOTE

github-actions bot commented Feb 1, 2022

ianbotsf left a comment • edited

Choose a reason for hiding this comment

aajtodd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 3, 2022

github-actions bot commented Feb 3, 2022

github-actions bot commented Feb 3, 2022

github-actions bot commented Feb 3, 2022

github-actions bot commented Feb 3, 2022

github-actions bot commented Feb 3, 2022

github-actions bot commented Feb 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aajtodd Feb 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aajtodd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 4, 2022

sonarcloud bot commented Feb 15, 2022

github-actions bot commented Feb 15, 2022

kggilmer commented Feb 1, 2022 •

edited

ianbotsf left a comment •

edited

aajtodd Feb 7, 2022 •

edited