Hello, First of all, thanks Stamatis for implementing this, I think it is something good for the project. In the beginning things might be a bit complicated (as always) and we might need some adjustments / clarifications, but I hope that in the long run we'll see this as a useful feature.
Regarding the two specific issues being discussed: - If I am not mistaken, the fact that SqlOperatorTest was moved out of 'test' was a consequence of [1], see the corresponding PR [2] "... it was necessary to move several classes from the 'core' module to 'testkit'". I don't know how simple (or how complex) a potential refactoring to move it out of there might be. Alternatively, it seems that this is rather an exceptional case, so perhaps it should qualify for an exception (e.g. everything under /testkit/* shall not be considered for coverage). - Regarding the instanceof, it seems that it was indeed a valid warning, and it has recently been fixed via [3] (see discussion on its PR [4]) Best regards, Ruben [1] https://issues.apache.org/jira/browse/CALCITE-4885 [2] https://github.com/apache/calcite/pull/2685 [3] https://github.com/apache/calcite/commit/4bebdb07c2f45a95c9a4fdf81e9bcfbdd11a15de [4] https://github.com/apache/calcite/pull/2919 On Tue, Jan 10, 2023 at 3:18 PM Stamatis Zampetakis <[email protected]> wrote: > Thanks for the feedback! > > I would like to stretch the fact that at this point it is at the discretion > of the reviewer/committer to enforce or ignore the information provided by > Sonar. > > Sonar, as other similar systems, has limitations thus there are always > going to be false positives. The rules/checks performed are fully > customisable so we can enable/disable them at will. > > The two issues highlighted by Julian seem like true positives to me. > * The "New code" that was introduced recently is not covered sufficiently > by tests and that's a fact. Part of the problem seems to come from the > recent modifications in SqlOperatorTest [1]. The class is located under > src/main (and not under src/test) so it is considered as a production class > and coverage checks are applied. There are ways to exclude certain paths > from coverage but I would argue that the class shouldn't be there in the > first place; we should probably log a CALCITE ticket for moving out of > there. > * The instance of warning is something that we probably don't want/cannot > fix (for the reasons mentioned in the PR) but Sonar did well to bring this > up; it is problematic to do comparisons by relying on the class name. > > I encourage others to share their thoughts/remarks as well so that we > improve as much as possible the developer experience. > > Best, > Stamatis > > [1] > > https://github.com/apache/calcite/blob/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java > > On Tue, Jan 10, 2023 at 1:38 AM Julian Hyde <[email protected]> wrote: > > > I see two false positives so far: > > * The message on be7135cf "58.1% Coverage on New Code (is less than > 80%)" > > * The bug on PR 2942 "Use an "instanceof" comparison instead" > > > > Has Sonarcube had any true positives yet? > > > > Vladimir used to hate when I was skeptical of changes to the build > > system. But I have no tolerance for a lint system that makes extra > > work. > > > > Julian > > > > > > > > > > On Mon, Jan 9, 2023 at 2:46 PM Francis Chuang <[email protected]> > > wrote: > > > > > > Thanks for implementing this, Stamatis! Having code quality metrics on > > > our repos is a huge win. > > > > > > Francis > > > > > > On 10/01/2023 4:22 am, Stamatis Zampetakis wrote: > > > > I just merged the CALCITE-5427 [1] enabling Sonarcloud analysis for > > Calcite > > > > main branch and new PRs. > > > > > > > > I have left the default Sonar quality gates active so you may start > > seeing > > > > Sonar reporting errors in various cases. > > > > > > > > If you encounter problems or you would like things to work > differently > > > > please create JIRA tickets and ping me if you need help. > > > > > > > > Once we are happy with how things work for Calcite we can also port > the > > > > changes to Avatica. > > > > > > > > Note that all Calcite committers have administrative privileges to > the > > > > Calcite project in Sonarcloud [2]. > > > > > > > > Best, > > > > Stamatis > > > > > > > > [1] > > > > > > > https://github.com/apache/calcite/commit/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959 > > > > [2] https://sonarcloud.io/project/roles?id=apache_calcite > > > > > > > > On Wed, Jan 4, 2023 at 5:44 PM Stamatis Zampetakis < > [email protected]> > > > > wrote: > > > > > > > >> The integration is advancing well and I am hoping to merge the PR in > > the > > > >> coming days. > > > >> To avoid unpleasant surprises, I am planning to create a new remote > > branch > > > >> in the main calcite repo to test some things out. I will delete it > as > > soon > > > >> as I am done with the tests. > > > >> > > > >> Best. > > > >> Stamatis > > > >> > > > >> On Wed, Dec 28, 2022 at 2:47 PM Michael Mior <[email protected]> > > wrote: > > > >> > > > >>> Thanks Stamatis! I haven't used SonarCloud before, but in general I > > think > > > >>> such tools can be quite useful. > > > >>> > > > >>> -- > > > >>> Michael Mior > > > >>> [email protected] > > > >>> > > > >>> > > > >>> On Sat, Dec 24, 2022 at 4:01 PM Stamatis Zampetakis < > > [email protected]> > > > >>> wrote: > > > >>> > > > >>>> Since there were no objections, I just logged INFRA-24038 [1] and > > plan > > > >>> to > > > >>>> move this forward. > > > >>>> > > > >>>> Let me know if you have questions or concerns regarding the > > adoption of > > > >>>> SonarCloud. > > > >>>> > > > >>>> Best, > > > >>>> Stamatis > > > >>>> > > > >>>> [1] https://issues.apache.org/jira/browse/INFRA-24038 > > > >>>> > > > >>>> On Sat, Dec 10, 2022 at 11:32 AM Benchao Li <[email protected] > > > > > >>> wrote: > > > >>>> > > > >>>>> Thanks Stamatis for bringing this up. > > > >>>>> > > > >>>>> I haven't used Sonar yet, but thanks for the demo[1] you > provided, > > it > > > >>>> looks > > > >>>>> really interesting. I think it's worth a try for Calcite. > > > >>>>> > > > >>>>> [1] https://github.com/zabetak/calcite/pull/9 > > > >>>>> > > > >>>>> Alessandro Solimando <[email protected]> > > 于2022年12月10日周六 > > > >>>>> 02:54写道： > > > >>>>> > > > >>>>>> Hi all, > > > >>>>>> thanks Stamatis for the proposal and the work, I am a huge fan > of > > > >>> Sonar > > > >>>>> and > > > >>>>>> I really miss it on Calcite, so a big +1 from me on this. > > > >>>>>> > > > >>>>>> In Hive so far we have received good feedback, so I hope it will > > be > > > >>>>>> welcomed here too. > > > >>>>>> > > > >>>>>> Best regards, > > > >>>>>> Alessandro > > > >>>>>> > > > >>>>>> On Fri, 9 Dec 2022 at 19:02, Stamatis Zampetakis < > > [email protected] > > > >>>> > > > >>>>>> wrote: > > > >>>>>> > > > >>>>>>> Hi all, > > > >>>>>>> > > > >>>>>>> I just logged CALCITE-5427 [1] for introducing code quality & > > > >>>> coverage > > > >>>>>>> metrics in Calcite CI. > > > >>>>>>> > > > >>>>>>> I added some motivation and examples under the ticket so please > > > >>> have > > > >>>> a > > > >>>>>> look > > > >>>>>>> and let me know what you think. > > > >>>>>>> > > > >>>>>>> If there are no objections, I will follow-up with INFRA to set > > > >>> things > > > >>>>> up > > > >>>>>>> for the official Calcite repo. > > > >>>>>>> > > > >>>>>>> The integration with SonarCloud has been inspired by HIVE-26196 > > > >>> [2] > > > >>>>> that > > > >>>>>>> Alessandro put in place for Hive and has been very helpful so > > far. > > > >>>>>>> > > > >>>>>>> Best, > > > >>>>>>> Stamatis > > > >>>>>>> > > > >>>>>>> [1] https://issues.apache.org/jira/browse/CALCITE-5427 > > > >>>>>>> [2] https://issues.apache.org/jira/browse/HIVE-26196 > > > >>>>>>> > > > >>>>>> > > > >>>>> > > > >>>>> > > > >>>>> -- > > > >>>>> > > > >>>>> Best, > > > >>>>> Benchao Li > > > >>>>> > > > >>>> > > > >>> > > > >> > > > > > > >
