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
Exclude junit4 so it stops getting accidentally imported #1122
Conversation
fa88de3
to
929f24c
Compare
@@ -39,6 +39,7 @@ ext.dep = [ | |||
"jettyWebsocketServer": "org.eclipse.jetty.websocket:websocket-server:9.4.18.v20190429", | |||
"jettyWebsocketServlet": "org.eclipse.jetty.websocket:websocket-servlet:9.4.18.v20190429", | |||
"jnrUnixsocket": "com.github.jnr:jnr-unixsocket:0.22", | |||
"junit4": "junit:junit:4.12", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
misk:misk
needs this because okhttp mock webserver needs it
import java.io.ByteArrayOutputStream | ||
import java.security.GeneralSecurityException | ||
|
||
@MiskTest(startService = true) | ||
class CryptoModuleTest { | ||
@Suppress("unused") | ||
@MiskTestModule | ||
val module = CryptoTestModule() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was broken for injection the whole time
@@ -37,7 +37,12 @@ internal class AssertExtensionsTest { | |||
} | |||
""") | |||
}).hasMessage( | |||
"""expected:<"{ "my_structure[]" : [ "this" , 45, "...> but was:<"{ "my_structure[2]" : [ "this" , 45, "...>""") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These messages are different in junit5
@@ -54,4 +54,5 @@ dependencies { | |||
testCompile dep.kotlinxCoroutines | |||
testCompile dep.mockitoCore | |||
testCompile project(':misk-testing') | |||
testImplementation dep.junit4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implementation
so we don't leak this dependency transitively
929f24c
to
b811831
Compare
I think using
implementation
also fixes this but that's a bigger change.People have been accidentally importing the wrong
@Test
which leads to injection stuff not getting set up which is a very confusing error message