Skip to content

[CALCITE-5611] Show sql for failed tests for SqlOperatorTest#3131

Closed
snuyanzin wants to merge 1 commit intoapache:mainfrom
snuyanzin:calcite5611
Closed

[CALCITE-5611] Show sql for failed tests for SqlOperatorTest#3131
snuyanzin wants to merge 1 commit intoapache:mainfrom
snuyanzin:calcite5611

Conversation

@snuyanzin
Copy link
Contributor

The PR is aiming to make failing test showing SQL for which the test is failed

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

86.7% 86.7% Coverage
0.0% 0.0% Duplication

@JiajunBernoulli
Copy link
Contributor

JiajunBernoulli commented Apr 2, 2023

The improvement is really helpful, but you changed many public interfaces.

I worry about they maybe cause damage to other users.

@snuyanzin Can you add interfaces instead of changing them?

@snuyanzin
Copy link
Contributor Author

hmm.. there are only 2 interfaces from testkit changed org.apache.calcite.sql.test.SqlTester.TypeChecker and org.apache.calcite.sql.test.SqlTester.ResultChecker
and both about matcher checkers....
Even if so the migration is pretty straightforward by adding String field

@rubenada
Copy link
Contributor

I agree with @snuyanzin , as far as I can see only two interfaces from testkit are modified, IMO that seems no big deal.

@JiajunBernoulli JiajunBernoulli added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Apr 19, 2023
@rubenada
Copy link
Contributor

LGTM. Needs to squash commits into a single one.

@snuyanzin
Copy link
Contributor Author

snuyanzin commented May 7, 2023

squashed commits and rebased

also changed String arg to Supplier<String> as mentioned by Julian in jira

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

86.7% 86.7% Coverage
0.0% 0.0% Duplication

@asfgit asfgit closed this in e92a5fc May 8, 2023
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.

4 participants