Re: [DISCUSS] Refactoring tests

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

Re: [DISCUSS] Refactoring tests

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

Re: [DISCUSS] Refactoring tests

I have merged my PR, and marked 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,

Re: [DISCUSS] Refactoring tests

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

Re: [DISCUSS] Refactoring tests

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

Re: [DISCUSS] Refactoring tests

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

Re: [DISCUSS] Refactoring tests

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 wrote: > FYI, I'm trying to do a thorough review today (as much

Re: [DISCUSS] Refactoring tests

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

Re: [DISCUSS] Refactoring tests

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

Re: [DISCUSS] Refactoring tests

I haven’t heard from anyone. I plan to merge https://issues.apache.org/jira/browse/CALCITE-4986 "Make HepProgram thread-safe” in the next hour or two, then I will squash/rebase/merge https://issues.apache.org/jira/browse/CALCITE-4885

Re: [DISCUSS] Refactoring tests

After a month or so of work I have a PR for review. See https://github.com/apache/calcite/pull/2685/ . The PR is huge (80 commits, 127 files changed, 20k lines added/removed) so may be hard to digest. I recommend that you read the commit log to

Re: [DISCUSS] Refactoring tests

Hello, I generally agree with the problems and goals set by CALCITE-4885 so definitely worth pushing this forward. I also like the ideas for being compatible with JUnit5 extensions etc., but I guess it could be something to address later one and does not need to be part of CALCITE-4885. If and

Re: [DISCUSS] Refactoring tests

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

Re: [DISCUSS] Refactoring tests

Julian, Thanks for taking time, and working on this. "Fixture" is a too broad term, and I do not mean we should replace it completely with AssertJ. What I mean is that AssertJ allows creating *discoverable* assertions. Hamcrest assertions are hard to create and hard to discover in IDE. --- Re

Re: [DISCUSS] Refactoring tests

I think it is a great idea to make the tests more accessible. A few comments to consider in your work (no response expected): - I would vote to try to minimize the use of non-interface class inheritance , especially in downstream cases. For example, I would avoid a dominant path using

[DISCUSS] Refactoring tests

I am working on https://issues.apache.org/jira/browse/CALCITE-4885, "Fluent test fixtures so that dependent projects can write parser, validator and rules tests". I wanted to give you all a heads up, because it's going to change quite a few lines of code. Here's the problem I'm trying to solve.