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: commits-unsubscr...@druid.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org