Skip to content

[CALCITE-6957] The RelOptRulesTest tests should fail if the xml file …#4321

Merged
NobiGo merged 1 commit intoapache:mainfrom
NobiGo:CALCITE-6957
Apr 20, 2025
Merged

[CALCITE-6957] The RelOptRulesTest tests should fail if the xml file …#4321
NobiGo merged 1 commit intoapache:mainfrom
NobiGo:CALCITE-6957

Conversation

@NobiGo
Copy link
Contributor

@NobiGo NobiGo commented Apr 18, 2025

…contains tests that do not exist in Java

@NobiGo NobiGo marked this pull request as draft April 18, 2025 17:05
@mihaibudiu
Copy link
Contributor

What's the relation between this and #4312?

@NobiGo
Copy link
Contributor Author

NobiGo commented Apr 18, 2025

What's the relation between this and #4312?

This PR can be directly replaced with #4312.

After this PR, we can solve the only existing XML methods by copying and pasting the file_actual.xml and don't need to delete them one by one.
In addition, the exception information will be clearer. For example, in PR's unit test DiffRepositoryTest#testMethodOnlyExistsInXml:
The exception will be:

java.lang.IllegalArgumentException: Actual and reference files differ. If you are adding new tests, replace the reference file with the current actual file, after checking its content.
diff /calcite/testkit/build/diffrepo/test/org/apache/calcite/test/DiffRepositoryTest_actual.xml /calcite/testkit/src/test/resources/org/apache/calcite/test/DiffRepositoryTest.xml
26d25
<   <TestCase name="testMethodOnlyExistsInXml1"/>

@NobiGo NobiGo marked this pull request as ready for review April 18, 2025 17:22
@xiedeyantu
Copy link
Member

This implementation is fantastic! If developers add tests in Java and XML formats, there will be no need to focus on case consistency issues. I will close my PR #4312 and switch to supporting this.

@NobiGo NobiGo added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Apr 19, 2025
@sonarqubecloud
Copy link

@NobiGo NobiGo merged commit 9be4b4e into apache:main Apr 20, 2025
21 of 36 checks passed
@NobiGo NobiGo deleted the CALCITE-6957 branch April 21, 2025 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants