[ 
https://issues.apache.org/jira/browse/CALCITE-5923?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17758578#comment-17758578
 ] 

Stamatis Zampetakis commented on CALCITE-5923:
----------------------------------------------

Apart from the behavior change that seems related to CALCITE-5678 we have to 
decide how we want the SqlOperatorTest to be extended.

According to CALCITE-4885 we should use SqlOperatorTest.fixture() to obtain a 
fixture and behavior can be changed by subclassing. However, with CALCITE-5557 
we added another extension point to change the fixtures behavior via 
parameterization. I don't think it is a good idea to keep both cause first it 
is easy to break things as it happened here and second it makes it harder to 
write and maintain these kind of tests.

Shall we keep only one extension point or not? WDYT? CC [~oliverlee] who added 
the parameterized tests in CALCITE-5557.

> Some test cases in `SqlOperatorTest` violates the test fixture's design 
> principle
> ---------------------------------------------------------------------------------
>
>                 Key: CALCITE-5923
>                 URL: https://issues.apache.org/jira/browse/CALCITE-5923
>             Project: Calcite
>          Issue Type: Bug
>          Components: core
>    Affects Versions: 1.35.0
>            Reporter: Runkang He
>            Assignee: Runkang He
>            Priority: Minor
>              Labels: pull-request-available
>
> There are some test cases in `SqlOperatorTest` directly use the 
> `SqlOperatorFixtureImpl.DEFAULT` to get the `SqlOperatorFixture`, including 
> `SqlOperatorTest.testCast` and many other test cases related with `CAST` 
> operator. This causes that the result check is missing when execute 
> `CalciteSqlOperatorTest`, which should has result check.
> This violates the design principle introduced by CALCITE-4885, which we 
> should alway use `SqlOperatorTest.fixture()` to get the `SqlOperatorFixture`. 
> This principle allows us to override`fixture()` method in subclasses to run 
> tests in a different environment.
> So I think we should fix these related test cases to keep consistent with the 
> principle.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

Reply via email to