capistrant commented on issue #9324: Fix failing Calcite unit tests with existing work-a-round for calcite property setup URL: https://github.com/apache/druid/pull/9324#issuecomment-583135063 @suneet-s I looked into this a little bit and I think I'm okay with Nishants approach to fix this with the `CalciteTestBase` class. It makes sense why he did it, since once the Calcite classes get loaded up and read the calcite specific System properties for configuration, they are stuck like that for the life of the JVM. This was an easy solve to make sure that the system properties were correct for the life of the JVM. It is not obvious to developers to add this unless they reference other tests that touch Calcite that they should extend his base class, which sucks. I'm not sure there is a way to configure Calcite without using System properties and Calcites.java seems to indicate the same in a from the comments in Calcites.setSystemProperties(). TravisCI passing despite all of this has me scratching my head. Maybe the tests are running in a different order than on local..? Or someone in the past messed with the configuration to get past this? I personally don't see the current approach as a problem. It seems like more of an inconvenience. However, if you have thoughts on a better solution, I think anyone would welcome it because what we currently have here stinks
---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
