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

Chore coroutine version bump #580

Merged
merged 11 commits into from
Feb 15, 2022
Merged

Chore coroutine version bump #580

merged 11 commits into from
Feb 15, 2022

Conversation

kggilmer
Copy link
Contributor

@kggilmer kggilmer commented Feb 1, 2022

Companion PR: awslabs/aws-sdk-kotlin#514

Issue #

awslabs/aws-sdk-kotlin#513

Description of changes

  • Update tests to use new coroutines-test APIs. See here for more details
  • 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

Copy link
Contributor

@ianbotsf ianbotsf left a comment

Choose a reason for hiding this comment

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

What's the scope for updating APIs that use integers as durations? The retry classes use integer milliseconds in StandardRetryStrategyOptions and ExponentialBackoffWithJitterOptions.

Copy link
Contributor

@aajtodd aajtodd left a comment

Choose a reason for hiding this comment

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

Nice! Other than the APIs Ian called out and a few questions looks great

@@ -25,6 +25,7 @@ kotlin {
commonTest {
dependencies {
implementation(project(":runtime:testing"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing we don't need this dependency in a lot of places anymore since most usages of it were to pull in runSuspendTest.

Not sure which projects are still using the other capabilities that remain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that there are none, as I removed them all and nothing broke... 🤷

import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertFails

@OptIn(ExperimentalCoroutinesApi::class)
Copy link
Contributor

Choose a reason for hiding this comment

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

question

What API is experimental that we have to opt-in to? (haven't played with the new APIs yet)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the new runTests provided by coroutines-test is experimental

@@ -95,11 +97,11 @@ class MutateHeadersTest {
// should leave in existing
assertEquals("qux", call.request.headers["baz"])

return@runSuspendTest
return@runTest
Copy link
Contributor

Choose a reason for hiding this comment

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

question

Do we even need these returns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't notice this. No, removed.

Comment on lines 139 to 141
"Took more than ${options.maxTimeMs}ms to yield a result",
"Took more than ${options.maxTime}ms to yield a result",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: The literal "ms" is now unnecessary. Duration.toString already formats units alongside values.

@sonarcloud
Copy link

sonarcloud bot commented Feb 15, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
1.2% 1.2% Duplication

@aajtodd aajtodd merged commit ff6a786 into main Feb 15, 2022
@aajtodd aajtodd deleted the chore-coroutine-version-bump branch February 15, 2022 18:55
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.

3 participants