abhishekrb19 commented on code in PR #16627:
URL: https://github.com/apache/druid/pull/16627#discussion_r1645243107
##########
services/src/test/java/org/apache/druid/cli/TestValidateIncompatibleCentralizedDatasourceSchemaConfig.java:
##########
@@ -51,6 +52,9 @@ public
TestValidateIncompatibleCentralizedDatasourceSchemaConfig(ServerRunnable
this.runnable = runnable;
}
+ // It seems that setting the system properties is causing MainTest to fail.
+ // Ignoring this test until there is a better way to set the properties.
+ @Ignore
@Test(expected = RuntimeException.class)
Review Comment:
I think this line is the problem. Looking at this test, an exception is
expected to be thrown when the runnable gets the system properties in L68-69.
After an exception is thrown, it doesn't clear the properties set from
L71-73 causing the incompatible system properties to be "leaked".
A couple of points regarding fixing this test:
1. Instead of setting system-wide properties that can affect other tests
broadly, you can bind the supplied properties to the Guice injector instance.
For example, please see
[SqlModuleTest.java](https://github.com/apache/druid/blob/master/sql/src/test/java/org/apache/druid/sql/guice/SqlModuleTest.java#L188).
This way, the properties would only be scoped to this test injector.
2. One of the problems with expected exceptions `@Test(expected =
Foo.class)` is that it doesn't tell us which line of code is expected to throw
the exception. We should use `Assert.assertThrows()` with a narrow code block
around lines 68-69.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
