I think sub-classing and parameterizing tests are both valid approaches. I agree that during review of 4952, parameterizing looked like the best option. But you had to add metadataConfig as a a parameter to lots of methods, which caused a massive conflict with my changes, and paving over your changes was the right approach.
Parameterized tests are probably best when the all parameter values are “similar". But they wouldn’t work well if, say, the parameter is a particular parser implementation and one parser implementation lives in a project that Calcite cannot even see. My ’test fixture’ approach works well for that scenario because in the dependent project you would sub-class the test to provide a modified fixture (with, say, a different parser and perhaps some other changes to configuration) and then the tests would all run on that fixture. A compromise approach might be for each test method to accept a fixture as the one and only argument. Then you could change other aspects of the fixture without having to refactor every test method. I did ensure that the same tests were being run before and after rebase. The biggest concern was whether the MetadataConfig is being set correctly in the tests. I had to create MetadataConfig.NOP and use it in 3 tests because otherwise the cluster’s metadata was being overwritten mid-way through the test. Different overloads of MetadataConfig.applyMetadata were called at different points within the test, and they would conflict. The NOP was a way to neutralize the conflicting calls. I’m not proud of that workaround, but I believe the necessary tests get run. Julian > On Jan 25, 2022, at 3:34 PM, Jacques Nadeau <jacq...@apache.org> wrote: > >> >> In your fix for https://issues.apache.org/jira/browse/CALCITE-4952 < >> https://issues.apache.org/jira/browse/CALCITE-4952> you made >> RelMetadataTest a parameterized test, so that each case could be called >> with two metadata providers. I undid that, and made it into a test with a >> protected fixture() method, so that a sub-class can run the same tests with >> a different provider. I believe that the test coverage is the same, albeit >> via different means. > > > Hmm. I actually started with the subclass pattern but after feedback during > review, Stamatis and I agreed that subclassing was an anti-pattern and thus > I went through the "fun" process of refactoring to use parameterization. > > In terms of confirming that the change to RelMetadataTest didn't disrupt > coverage, any chance you ran a diff on test output pre/post to confirm that > at least all the same test cases are running? (That's a really hard giant > file diff to compare given all the refactoring.) > > > > On Sat, Jan 22, 2022 at 9:41 PM Julian Hyde <jhyde.apa...@gmail.com> wrote: > >> Gavin, Thanks for the kind words. >> >> All, I have now squashed and rebased onto latest master. The squashed >> commit is >> https://github.com/julianhyde/calcite/commit/cb59231a72e23be260b21670012af33c47c2610e. >> I plan to merge to master on Monday. >> >> Jacques, You may wish to carefully review my changes to RelMetadataTest. >> In your fix for https://issues.apache.org/jira/browse/CALCITE-4952 < >> https://issues.apache.org/jira/browse/CALCITE-4952> you made >> RelMetadataTest a parameterized test, so that each case could be called >> with two metadata providers. I undid that, and made it into a test with a >> protected fixture() method, so that a sub-class can run the same tests with >> a different provider. I believe that the test coverage is the same, albeit >> via different means. >> >> Julian >> >> >>> On Jan 22, 2022, at 9:53 AM, Gavin Ray <ray.gavi...@gmail.com> wrote: >>> >>> Thank you for doing this, after reading the overview these changes seem >>> like they will make a number of things easier related to testing. >>> Super useful too when you're trying to start building something with >>> Calcite but don't know it well enough to figure out how to put all the >>> pieces together yourself. >>> >>> On Fri, Jan 21, 2022 at 3:57 PM Julian Hyde <jhyde.apa...@gmail.com> >> wrote: >>> >>>> If it helps you review, I have created a ’summary’ document with a >>>> description of the changes. It will become the commit message when I >>>> squash, rebase, and merge to master. >>>> >>>> >>>> >> https://github.com/julianhyde/calcite/blob/4885-test-fixtures/summary-of-calcite-4885.md >>>> < >>>> >> https://github.com/julianhyde/calcite/blob/4885-test-fixtures/summary-of-calcite-4885.md >>> >>>> >>>> >>>> Julian >>>> >>>> >>>>> On Jan 19, 2022, at 9:08 PM, Jacques Nadeau <jacq...@apache.org> >> wrote: >>>>> >>>>> Unfortunately, the last minute attendance of the meetup today threw my >>>>> schedule off and I won't be able to look at this for at least a few >> days. >>>>> Don't feel obligated to hold up for me. >>>>> >>>>> On Wed, Jan 19, 2022, 9:04 AM Jacques Nadeau <jacq...@apache.org> >> wrote: >>>>> >>>>>> FYI, I'm trying to do a thorough review today (as much as possible >> with >>>>>> patch this size). >>>>>> >>>>>> On Wed, Jan 19, 2022, 4:36 AM Michael Mior <mm...@apache.org> wrote: >>>>>> >>>>>>> Thanks for doing this Julian! I haven't been able to do a detailed >>>> review, >>>>>>> but from my skim of the PR, this looks like a nice improvement. I >> think >>>>>>> it's always been a bit difficult for new Calcite developers to write >>>> good >>>>>>> tests, especially for new modules. So anything that helps that >>>> experience >>>>>>> is very welcome. >>>>>>> >>>>>>> -- >>>>>>> Michael Mior >>>>>>> mm...@apache.org >>>>>>> >>>>>>> >>>>>>> Le mer. 17 nov. 2021 à 01:15, Vladimir Sitnikov < >>>>>>> sitnikov.vladi...@gmail.com> >>>>>>> a écrit : >>>>>>> >>>>>>>> Jacques>This sounds like it will mean we will need to make >>>> calcite-core >>>>>>>> test artifacts available >>>>>>>> >>>>>>>> Test artifacts publication is yet another anti-pattern just like >> "base >>>>>>> test >>>>>>>> class". >>>>>>>> This change has been discussed: >>>>>>>> https://lists.apache.org/thread/fz96p94h016p11g777otqntjxg2oxgh1 >>>>>>>> >>>>>>>> If you want to depend on a class from tests, consider moving it to >>>>>>> /testkit >>>>>>>> module: >>>>>>>> >>>>>>>> >>>>>>> >>>> >> https://github.com/apache/calcite/tree/0899e6c157632ba1c5369a942cfe2be15fb4ed9f/testkit >>>>>>>> >>>>>>>> Jacques>We should think about the rules around Kotlin >>>>>>>> >>>>>>>> What happens in calcite-core/tests stays in calcite-core/tests :) >>>>>>>> >>>>>>>> It is reasonable to assume that testkit module would have >>>> dependencies, >>>>>>>> and testkit would provide API that is usable from Java and other JVM >>>>>>>> languages. >>>>>>>> >>>>>>>> In that regard, Kotlin dependency in testkit is not much different >>>> from >>>>>>>> Quidem or commons-lang3. >>>>>>>> Consumers might use Quidem if it fits just like they could use >> Kotlin >>>>>>> if it >>>>>>>> fits. >>>>>>>> >>>>>>>> Vladimir >>>>>>>> >>>>>>> >>>>>> >>>> >>>> >> >>
