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

Allow constructor injection of MockWebServer #8191

Merged
merged 6 commits into from
Jan 14, 2024

Conversation

yschimke
Copy link
Collaborator

No description provided.

@yschimke yschimke marked this pull request as ready for review January 13, 2024 13:28
@yschimke
Copy link
Collaborator Author

yschimke commented Jan 13, 2024

@swankjesse interestingly, some failures on an earlier build were familiar flaky tests. maybe a clue on why it's failing.

https://github.com/square/okhttp/actions/runs/7512760479/job/20453782860

HttpOverHttp2Test > recoverFromCancelReusesConnection(Protocol, MockWebServer) > [2] h2 FAILED
    java.lang.IllegalStateException: shutdown() already called
        at mockwebserver3.MockWebServer.start(MockWebServer.kt:332)
        at mockwebserver3.MockWebServer.start(MockWebServer.kt:322)
        at mockwebserver3.MockWebServer.start(MockWebServer.kt:309)

@yschimke yschimke requested a review from swankjesse January 13, 2024 17:22
@yschimke yschimke added the android Relates to usage specifically on Android label Jan 13, 2024
@yschimke
Copy link
Collaborator Author

yschimke commented Jan 13, 2024

Will test on android as I think that's why we needed the extend with.

resolved: "configurationParameters" to "junit.jupiter.extensions.autodetection.enabled=true"

@yschimke yschimke merged commit a16dcec into square:master Jan 14, 2024
21 checks passed
Copy link
Collaborator

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -20,7 +20,8 @@ android {
testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner"
testInstrumentationRunnerArguments += mapOf(
"runnerBuilder" to "de.mannodermaus.junit5.AndroidJUnit5Builder",
"notPackage" to "org.bouncycastle"
"notPackage" to "org.bouncycastle",
"configurationParameters" to "junit.jupiter.extensions.autodetection.enabled=true"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do our end-users need to do this? JUnit 5 is such a mess.

Copy link

@TWiStErRob TWiStErRob Oct 4, 2024

Choose a reason for hiding this comment

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

don't "need to", users have the option of

  • using the extension directly by annotating test class.
  • using the extension directly by annotating an annotation to collect extensions
  • setting this property to magically apply extension to all tests

They must consume extensions somehow for sure, but this is not much different than adding a rule to a class in JUnit 4, right?

Copy link

@TWiStErRob TWiStErRob Oct 4, 2024

Choose a reason for hiding this comment

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

Oh, just realized that this is in internal package, but it's public API, as users can choose how to use the extensions (you choose to use the atuodetection in this project). Could this be made actual API? @swankjesse

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should change our MockWebServer JUnit 5 extension API to use an annotation on each annotated class. It’s two imports instead of JUnit 4’s single import, but it’s self-contained in the test class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android Relates to usage specifically on Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants