I have merged my PR, and marked https://issues.apache.org/jira/browse/CALCITE-4885 <https://issues.apache.org/jira/browse/CALCITE-4885> fixed.
If my changes make it difficult to rebase, I apologize. If you need explanation for the changes I made, check the (very detailed) commit message, browse the un-squashed commits in https://github.com/julianhyde/calcite/tree/4885-test-fixtures.0 <https://github.com/julianhyde/calcite/tree/4885-test-fixtures.0>, or ask for my help on this thread. Julian > On Jan 22, 2022, at 9:41 PM, Julian Hyde <[email protected]> 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 > > <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] >> <mailto:[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] >> <mailto:[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> >>> < >>> 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 >>>>>>> >>>>>> >>>>> >>> >>> >
