Skip to content

[CALCITE-5340] Tests should fail when actual and expected XML reference files are not identical#2948

Merged
asolimando merged 1 commit intoapache:mainfrom
asolimando:main-CALCITE-5340-fail_on_diff_xml_reference
Oct 26, 2022
Merged

[CALCITE-5340] Tests should fail when actual and expected XML reference files are not identical#2948
asolimando merged 1 commit intoapache:mainfrom
asolimando:main-CALCITE-5340-fail_on_diff_xml_reference

Conversation

@asolimando
Copy link
Member

@asolimando asolimando commented Oct 22, 2022

No description provided.

@asolimando asolimando added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Oct 24, 2022
@libenchao
Copy link
Member

libenchao commented Oct 25, 2022

@asolimando The PR looks great. The only question I got is why TopDownOptTest and RuleMatchVisualizerTest are not touched in this PR?

@asolimando
Copy link
Member Author

asolimando commented Oct 25, 2022

@asolimando The PR looks great. The only question I got is why TopDownOptTest and RuleMatchVisualizerTest are not touched in this PR?

@libenchao, thanks a lot for catching it! I totally missed TopDownOptTest, for RuleMatchVisualizerTest I tried to implement the check in its parent class (RelOptTestBase) but it proved difficult, I gave up and forgot about that class.

I have covered them both in a new commit, please have another look if you have any spare cycles.

@libenchao
Copy link
Member

libenchao commented Oct 25, 2022

Sorry that I found another one: SqlPrettyWriterTest.

libenchao
libenchao approved these changes Oct 25, 2022
View reviewed changes
Copy link
Member

@libenchao libenchao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @asolimando thanks for driving this, this is very helpful improvement.

@asolimando
Copy link
Member Author

asolimando commented Oct 25, 2022

LGTM. @asolimando thanks for driving this, this is very helpful improvement.

Thanks to you, @libenchao, for your reviews and helpful suggestions!

@asolimando
Copy link
Member Author

asolimando commented Oct 25, 2022

Squashing before merging.

@asolimando asolimando force-pushed the main-CALCITE-5340-fail_on_diff_xml_reference branch from 02299f2 to 3d3a121 Compare October 25, 2022 15:41
@asolimando
[CALCITE-5340] Tests should fail when actual and expected XML referen…
6f861de 
…ce files are not identical

File check added to the following test suites:
- HepPlannerTest
- RelOptRulesTest
- RuleMatchVisualizerTest
- SqlHintsConverterTest
- SqlToRelConverterTest
- SqlPrettyWriterTest
- TypeCoercionConverterTest
- TopDownOptTest

Updated XML test reference files:
- HepPlannerTest.xml
- RelOptRulesTest.xml
- SqlHintsConverterTest.xml
- SqlToRelConverterTest.xml
- TypeCoercionConverterTest.xml
@asolimando asolimando force-pushed the main-CALCITE-5340-fail_on_diff_xml_reference branch from 3d3a121 to 6f861de Compare October 26, 2022 08:50
@asolimando asolimando merged commit f0c6cd5 into apache:main Oct 26, 2022
@asolimando asolimando deleted the main-CALCITE-5340-fail_on_diff_xml_reference branch October 26, 2022 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@libenchao libenchao libenchao approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@asolimando @libenchao