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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 >>>>> [email protected] >>>>> >>>>> >>>>> Le mer. 17 nov. 2021 à 01:15, Vladimir Sitnikov < >>>>> [email protected]> >>>>> 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 >>>>>> >>>>> >>>> >> >>
