You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "tanclary (via GitHub)" <gi...@apache.org> on 2024/01/04 22:57:38 UTC
[PR] [CALCITE-6172] Allow aliased operators to re-use existing tests [calcite]
tanclary opened a new pull request, #3612:
URL: https://github.com/apache/calcite/pull/3612
I used @snuyanzin 's PoC PR and @julianhyde 's improvements to make this PR and have credited them in the latest commit in this chain.
I did not modify MOD and % because the syntax seemed different enough that I preferred keeping them separate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6172] Allow aliased operators to re-use existing tests [calcite]
Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on PR #3612:
URL: https://github.com/apache/calcite/pull/3612#issuecomment-1877965446
> IMHO the issue is that this test (check for capital letter at the beginning) makes most or even all the tests are failing here... It's better to see that tests are still green before approval...
Ahh gotcha, I've fixed this. Thanks for pointing it out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6172] Allow aliased operators to re-use existing tests [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3612:
URL: https://github.com/apache/calcite/pull/3612#issuecomment-1881598726
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3612) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3612&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3612&resolved=false&inNewCodePeriod=true)
[97.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3612&metric=new_coverage&view=list)
[18.6% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3612&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3612)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6172] Allow aliased operators to re-use existing tests [calcite]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #3612:
URL: https://github.com/apache/calcite/pull/3612#issuecomment-1879015198
ok, thanks for clarification
then I think we don't need a javadoc
```java
/** Contains alias data for parameterized tests. */
```
or at least need to change it since it's talking about non existing parameterized tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6172] Allow aliased operators to re-use existing tests [calcite]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #3612:
URL: https://github.com/apache/calcite/pull/3612#issuecomment-1878344968
thanks
I noticed that currently there is `FunctionAlias` for parameterized tests however there is no junit parameterized tests using it, is it intentional?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6172] Allow aliased operators to re-use existing tests [calcite]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #3612:
URL: https://github.com/apache/calcite/pull/3612#issuecomment-1877961133
IMHO the issue is that this test (check for capital letter at the beginning) makes most or even all the tests are failing here... It's better to see that tests are still green before approval...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6172] Allow aliased operators to re-use existing tests [calcite]
Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on PR #3612:
URL: https://github.com/apache/calcite/pull/3612#issuecomment-1877956975
> Thanks for driving this Seems you need to change one of your commit messages since there is a test checking that every commit message starts with a capital letter https://github.com/apache/calcite/actions/runs/7415936383/job/20179990922?pr=3612#step:4:642
Yeah after y'all approved I was going to squash down to the most recent commit that has a valid message if that's alright.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6172] Allow aliased operators to re-use existing tests [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3612:
URL: https://github.com/apache/calcite/pull/3612#issuecomment-1881572078
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3612) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3612&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3612&resolved=false&inNewCodePeriod=true)
[97.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3612&metric=new_coverage&view=list)
[18.6% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3612&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3612)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6172] Allow aliased operators to re-use existing tests [calcite]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #3612:
URL: https://github.com/apache/calcite/pull/3612#issuecomment-1877950056
Thanks for driving this
Seems you need to change one of your commit messages since there is a test checking that every commit message starts with a capital letter
https://github.com/apache/calcite/actions/runs/7415936383/job/20179990922?pr=3612#step:4:642
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6172] Allow aliased operators to re-use existing tests [calcite]
Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary merged PR #3612:
URL: https://github.com/apache/calcite/pull/3612
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6172] Allow aliased operators to re-use existing tests [calcite]
Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on PR #3612:
URL: https://github.com/apache/calcite/pull/3612#issuecomment-1878988739
> thanks
>
> I noticed that currently there is `FunctionAlias` for parameterized tests however there is no junit parameterized tests using it, is it intentional?
@snuyanzin Julian suggested still giving each operator its own test method and then having aliases use a common method for general tests. I think this makes it a bit more readable and makes it easier to add tests for a specific alias should the need arise. What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6172] Allow aliased operators to re-use existing tests [calcite]
Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on PR #3612:
URL: https://github.com/apache/calcite/pull/3612#issuecomment-1879063071
@snuyanzin addressed your comment in latest commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org