Thanks Alessandro for driving this. It's great to hear this is being improved. I suffered from this problem several times before.
Alessandro Solimando <[email protected]> 于2022年10月24日周一 17:25写道： > 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 < > [email protected]> 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 <[email protected]> > > 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 <[email protected]> > >> 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 < > >> > [email protected]> 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 > >> > > >> > > >> > > > -- Best, Benchao Li
