I've got no objections. Appreciate you adding coverage support. On Thu, Jun 21, 2018, 10:06 PM Marco de Abreu <[email protected]> wrote:
> Tianqi and Kellen, do you mind if we move ahead and merge > https://github.com/apache/incubator-mxnet/pull/11344 now? > > -Marco > > On Thu, Jun 21, 2018 at 4:50 PM Yasser Zamani <[email protected]> > wrote: > > > +1 but just as an MXNet lover newbie :) > > > > We at Apache Struts, already use "coveralls" and I personally like it a > > lot even with some limitations with it. It doesn't allow us to merge a > > PR with a not tested code. It reports us where are those not covered new > > codes. > > > > Regards. > > > > On 6/21/2018 7:13 PM, Marco de Abreu wrote: > > > To avoid any confusion, I have disable the PR comments at > > > > > > https://github.com/apache/incubator-mxnet/pull/11344/files#diff-1f7b4b85ed8525c5239f741431a72872R25 > > . > > > Thus, the data will only be recorded in the background is then > > retrievable > > > at https://codecov.io/gh/apache/incubator-mxnet/pulls. > > > > > > -Marco > > > > > > On Thu, Jun 21, 2018 at 6:43 AM Marco de Abreu < > > [email protected]> > > > wrote: > > > > > >> Hi Kellen, > > >> > > >> Thanks for your feedback. > > >> > > >> The same argument about a hosted service could be applied to GitHub - > we > > >> could just use the git repository hosted under Apache Infra then and > > >> completely stick to JIRA. This service gives us the advantage of being > > free > > >> of charge, a mature project and having built-in support for all > > languages > > >> we run in this project. If you got an open source software which > offers > > the > > >> same features and is as easy to manage, I'm happy to take suggestions. > > >> > > >> As stated previously, we're going to start with Python and C++ > coverage, > > >> evaluate it over the following days/weeks and then discuss how we > should > > >> move forward. I'm happy to take contributions from the community to > > extend > > >> the coverage across more languages. For now, I'd just like to gather > > some > > >> data. > > >> > > >> Best regards, > > >> Marco > > >> > > >> > > >> kellen sunderland <[email protected]> schrieb am Do., 21. > > Juni > > >> 2018, 11:15: > > >> > > >>> -0.1 (non-binding). > > >>> > > >>> Looks good, and I like the idea of adding coverage, however I have a > > few > > >>> minor issues with this approach: > > >>> > > >>> First it seems to use a hosted service, which can disappear / be > > acquired > > >>> / > > >>> change terms at any time. In general I would try and avoid using > > services > > >>> like this for Apache projects. I don't feel as strongly about this > as > > >>> some > > >>> in the community do, but there are so many open source alternatives > to > > >>> generate coverage reports that I'm not sure why we have to go the > Saas > > >>> route in this case. > > >>> > > >>> Secondly the core library is written in C++. Most of the significant > > bugs > > >>> I've seen in MXNet so far have been in the C++ code. I'd suggest we > > focus > > >>> on coverage reports for C++ before, or at least in addition to > > Python. I > > >>> feel less strongly about the other languages, but coverage in other > > areas > > >>> (i.e. Scala) might also be nice eventually. > > >>> > > >>> -Kellen > > >>> > > >>> On Thu, Jun 21, 2018 at 4:59 AM Chris Olivier <[email protected] > > > > >>> wrote: > > >>> > > >>>> we should have a betting pool on what the current coverage is. > > >>>> > > >>>> On Wed, Jun 20, 2018 at 7:54 PM Naveen Swamy <[email protected]> > > >>> wrote: > > >>>> > > >>>>> +1 to collect data on coverage. > > >>>>> > > >>>>> On Wed, Jun 20, 2018 at 7:38 PM, Marco de Abreu < > > >>>>> [email protected]> wrote: > > >>>>> > > >>>>>> Hello, > > >>>>>> > > >>>>>> for now, this is only a test for myself and a way to gather data > in > > >>>> order > > >>>>>> to give us the possibility to make a data-driven decision. So far, > > >>> we > > >>>>> have > > >>>>>> not decided on the implications and which restrictions we will set > > >>> as > > >>>>>> result for the produced metrics and I'd like to postpone that > until > > >>> we > > >>>>> know > > >>>>>> the confidence of the system. This impact is one of the aspects > I'd > > >>>> like > > >>>>> to > > >>>>>> assess in the next weeks. > > >>>>>> > > >>>>>> The initial goal is to give the reviewer an additional tool to > > >>> assess > > >>>> the > > >>>>>> coverage of one's PR. One example could be the optimization of > some > > >>>>> backend > > >>>>>> logic. The reviewer would then see if the changed codeis actually > > >>> being > > >>>>> hit > > >>>>>> by any tests or if they are being missed entirely. I agree that > just > > >>>> the > > >>>>>> percentage could be highly misleading and we should review that > very > > >>>>>> clearly. > > >>>>>> > > >>>>>> For now, I'd only like to gather the data in the background. I > would > > >>>>> follow > > >>>>>> up with a big thread on dev@ about my observations, my > > >>> recommendations > > >>>>> and > > >>>>>> the possible options we have to use the data. We can then decide > as > > >>>>>> community how exactly we would like to proceed and how much > > >>> importance > > >>>> we > > >>>>>> give to the generated reports. > > >>>>>> > > >>>>>> Best regards, > > >>>>>> Marco > > >>>>>> > > >>>>>> > > >>>>>> > > >>>>>> On Wed, Jun 20, 2018 at 7:23 PM Tianqi Chen < > > >>> [email protected]> > > >>>>>> wrote: > > >>>>>> > > >>>>>>> While I think test coverage is a nice information to have. I > would > > >>>>> object > > >>>>>>> to using this as a metric to decide whether a PR should be > merged. > > >>>>>>> Code-cov act as a mere coverage of APIs, which is a useful > > >>> aspect, it > > >>>>> can > > >>>>>>> be misleading in many cases, especially when such change involves > > >>>>>>> cross-language APIs and automatically generated wrapper. > > >>>>>>> Sometimes the code-cov shows a negative impact on coverage while > > >>> CI > > >>>>>> passes, > > >>>>>>> and the giving information was quite misleading if you just look > > >>> at > > >>>> the > > >>>>>> PR > > >>>>>>> tabs > > >>>>>>> > > >>>>>>> I would still trust the reviewer's decision on the pull request > > >>>>> merging. > > >>>>>>> > > >>>>>>> Tianqi > > >>>>>>> > > >>>>>>> On Wed, Jun 20, 2018 at 7:14 PM, Marco de Abreu < > > >>>>>>> [email protected]> wrote: > > >>>>>>> > > >>>>>>>> Hello, > > >>>>>>>> > > >>>>>>>> I'd like to introduce test coverage metrics of PRs using > > >>>>>>>> https://codecov.io/. > > >>>>>>>> This tool will aggregate coverage reports across multiple runs, > > >>>>>> platforms > > >>>>>>>> and technologies and gives contributors as well as reviewers a > > >>> new > > >>>>> tool > > >>>>>>>> that allows to improve the quality of a pull request. > > >>>>>>>> > > >>>>>>>> Since we need to gather some data first, I'd like to request > > >>>> merging > > >>>>>>>> https://github.com/apache/incubator-mxnet/pull/11344. This will > > >>>>> enable > > >>>>>>>> publishing the coverage data to the service and have no impact > > >>> on > > >>>>> your > > >>>>>>> PRs > > >>>>>>>> - it will just allow me to assess the quality of the service in > > >>> the > > >>>>>>>> background while I come up with a full integration design. My > > >>>> initial > > >>>>>>> plan > > >>>>>>>> is to start with coverage across Python and C++ and then, with > > >>> the > > >>>>> help > > >>>>>>> of > > >>>>>>>> our community, extend the report across all our supported > > >>>> languages. > > >>>>>>>> > > >>>>>>>> Does anybody object to having us gather this data in the > > >>>> background? > > >>>>>>>> > > >>>>>>>> Best regards, > > >>>>>>>> Marco > > >>>>>>>> > > >>>>>>> > > >>>>>> > > >>>>> > > >>>> > > >>> > > >> > > > > > >
