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