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 > > > > >
