I have a working implementation in the PR 2948 <https://github.com/apache/calcite/pull/2948> for CALCITE-5340 <https://issues.apache.org/jira/browse/CALCITE-5340>, it's covering the following test suites:
- HepPlannerTest - RelOptRulesTest - SqlHintsConverterTest - SqlToRelConverterTest - TypeCoercionConverterTest The associated XML reference files will be updated in the context of this PR, and from now on the tests will fail upon discrepancies. Happy to hear your opinion if you have time. Best regards, Alessandro On Fri, 21 Oct 2022 at 10:42, Alessandro Solimando < alessandro.solima...@gmail.com> wrote: > Thank you guys for your input. > > Given that the official way to update the XML file (as per our doc) is to > automatically generate it, I think it should fail if the actual and > expected files differ, whatever the difference is. > > I will dig more into how DiffRepository works and make the test fail on > discrepancies in a subsequent PR. > > In the meantime, I have filed CALCITE-5340 > <https://issues.apache.org/jira/browse/CALCITE-5340> to track this. > > Best regards, > Alessandro > > On Fri, 21 Oct 2022 at 10:16, Stamatis Zampetakis <zabe...@gmail.com> > wrote: > >> Thanks for taking care of this Alessandro! >> >> I believe the resources are still sorted as per [1] but it appears that >> there are still a few manual edits to the file. >> >> If we want to prevent such changes in the future it may be a good idea to >> add a post-class check In RelOptRulesTest (or somewhere in DiffRepo) >> checking that files are identical. >> >> Best, >> Stamatis >> >> [1] https://lists.apache.org/thread/j0fh9hhxdc9lc7n228kwj6xcs0z9hpb3 >> >> On Thu, Oct 20, 2022 at 8:36 PM Julian Hyde <jhyde.apa...@gmail.com> >> wrote: >> >> > Do you think that RelOptRulesTest should fail if the resources are not >> > sorted alphabetically? (I thought it did that at some point… I may be >> > wrong. Anyway, I am a fan of ‘fail fast’. The person who introduces the >> > error should be the person to fix it.) >> > >> > > On Oct 20, 2022, at 9:20 AM, Alessandro Solimando < >> > alessandro.solima...@gmail.com> wrote: >> > > >> > > Hello everyone, >> > > I have recently added tests to RelOptRulesTest.java, took the chance >> to >> > > update (maven instruction with gradle instructions, path has changed) >> and >> > > improved them a little bit. >> > > >> > > While testing the commands I have noticed that few PRs in the last >> months >> > > had tests added to RelOptRulesTest.java with a manual update of >> > > RelOptRulesTest.xml (possibly due to the stale instructions). >> > > >> > > Manual updates often suffer from formatting issues, stale/wrong SQL >> > > statements, different test order. >> > > >> > > Note that those discrepancies are not caught by running tests/CI, but >> > it's >> > > nonetheless problematic, especially because it forbids folks down the >> > line >> > > to re-generate the file automatically, because their PRs would carry >> the >> > > changes from past work. >> > > >> > > In https://github.com/apache/calcite/pull/2944 I will put back on >> track >> > > RelOptRulesTest.xml, please try to follow the procedure in the >> javadoc of >> > > RelOptRulesTest.java and let me know if we can improve them even >> further >> > in >> > > your opinion. >> > > >> > > Best regards, >> > > Alessandro >> > >> > >> >
