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

Switch to assertFailsWith #8177

Merged
merged 10 commits into from
Jan 6, 2024
Merged

Switch to assertFailsWith #8177

merged 10 commits into from
Jan 6, 2024

Conversation

swankjesse
Copy link
Collaborator

No description provided.

fail<Any>("Second connection should be refused")
} catch (e: ConnectException) {
assertThat(e.message!!).contains("refused")
}.also { expected ->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This idiom isn’t perfect, but it was easy enough to Structurally Replace and it makes the diffs pretty easy to review

} catch (e: ConnectException) {
assertThat(e.message!!).contains("refused")
}.also { expected ->
assertThat(expected.message!!).contains("refused")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also renamed e to expected wherever I could

@@ -21,6 +21,7 @@ import assertk.assertions.hasMessage
import assertk.assertions.isEmpty
import assertk.assertions.isEqualTo
import java.io.IOException
import kotlin.test.assertFailsWith
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I’m using assertFailsWith instead of assertFailure because assertFailure needs a 2nd assertion to specify the exception type

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I argued for an overload that took a reified exception type when I added assertFailure and lost (or, well, didn't press it too hard when given push-back).

Maybe file a feature request and we can try again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines 23 to +25
testImplementation(libs.kotlin.test.annotations)
testImplementation(libs.kotlin.test.common)
testImplementation(libs.kotlin.test.junit)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The need for all three of these is very strange. You should really only need "common" as the Gradle module metadata will resolve both the annotations and the junit implementation automatically in almost all cases.

There are a few exceptions, maybe in the MWS modules, where you have to be explicit. But 99% of modules should get the automatic behavior.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I’m frustrated with this one. For reasons unclear to me omitting libs.kotlin.test.junit causes compile to fail.

> Task :okhttp-brotli:compileTestKotlin FAILED
e: file:///Volumes/Development/okhttp/okhttp-brotli/src/test/java/okhttp3/brotli/BrotliInterceptorTest.kt:24:20 Unresolved reference: assertFailsWith

@JakeWharton
Copy link
Collaborator

JakeWharton commented Jan 6, 2024

Saw some out-of-order imports. Probably need a spotApply run.

@swankjesse
Copy link
Collaborator Author

The whole repo needs some Spotless love. It’s currently misconfigured and broken. I’ve got that on my queue for demultiplatform cleanup.

@swankjesse swankjesse merged commit 23d67c3 into master Jan 6, 2024
19 checks passed
@swankjesse swankjesse deleted the jwilson.0105.assertFailsWith branch January 6, 2024 05:31
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