kfaraz commented on PR #13106: URL: https://github.com/apache/druid/pull/13106#issuecomment-1260415405
Haven't reviewed the PR thoroughly yet but it seems pretty useful and would make test cases much more readable. A couple of first glance comments: - I suppose the only use of `TestCaseWriter` would be to convert existing tests as new tests would always be written in the new format. I suppose you would call this writer from somewhere inside `BaseCalciteQueryTest.testQuery()`, run all the tests and print out all the test cases in one go. Please correct me if I am missing something. - I would advise using yaml rather than having to define a whole new syntax. Yaml is meant to tackle such requirements, you can easily have multi-line strings, even embed json data, without compromising on the format or having to unnecessarily quote things. - I would also advise removing the suffix `...Section` from different objects that make up a test case. I feel that the decision to write things in a "section" or otherwise is really an impl detail. It would be cognitively simpler if a test case just specifies `Resources`, `Context`, `ExpectedResults` etc., being completely agnostic of the underlying file format. -- 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]
