Change lgtm. Merged. You are unblocked for updating the jenkins job. Please remember deleting the wip jenkins job after you update the jenkins dsl.
- Sijie On Fri, Feb 23, 2018 at 1:12 AM, Enrico Olivelli <eolive...@gmail.com> wrote: > Patch is ready forto review > https://github.com/apache/bookkeeper/pull/1129 > > Once all is working okay in master I will submit patch for standard jenkins > job > > Il lun 19 feb 2018, 10:45 Sijie Guo <guosi...@gmail.com> ha scritto: > > > On Mon, Feb 19, 2018 at 12:23 AM, Enrico Olivelli <eolive...@gmail.com> > > wrote: > > > > > News: > > > I am struggling to create an aggregate code coverage report, but > > actually I > > > have troubles with (I suspect) PowerMock, which is changing classes at > > > runtime. > > > > > > The goal of an aggregate report is to consider in the report test cases > > > which "test" classes which are not bundled within the same maven > project. > > > > > > After running thru the codebase many times I have some points: > > > - there is no much value in having unit tests which test code outside > the > > > same module and it is a bad practice > > > - bookkeeper does not contain many cases (I cannot directly find any, > > but I > > > did not spend time on this), I think that most of the cases are about > > > classes moved to utility modules. > > > > > > Given these points I think we can step over the 'aggregate code > coverage > > > report" and move forward with 'classic' code coverage reporting for > > > Maven/JaCoCo: each module report must be self contained, > > > With this approach we are at 72% code coverage (given Coverall.io KPIs) > > and > > > there is already much work to do in order to tidy up our codebase. > > > > > > So my proposal: > > > - setup a definitive code coverage system, with classic code-coverage > > > report > > > - tidy up (during 4.8) > > > - when we are okay we can see if there is real need to introduce > > > multi-project report (I hope it won't be needed after tidying up) > > > > > > If my proposal is good I will clean up the patch: > > > - remove aggregate reporting > > > - enable the report on DL part of code base > > > > - submit patch for review > > > > > > Then once the patch is merged: > > > - setup final CI test jobs > > > - submit patch for final Jenkins CI jobs using Jenkins DSL > > > > > > Thoughts ? > > > > > > > sgtm. > > > > > > > > > > > > > Enrico > > > > > > 2018-02-15 13:47 GMT+01:00 Sijie Guo <guosi...@gmail.com>: > > > > > > > ack > > > > > > > > On Thu, Feb 15, 2018 at 2:15 PM, Enrico Olivelli < > eolive...@gmail.com> > > > > wrote: > > > > > > > > > Il mer 14 feb 2018, 23:35 Sijie Guo <guosi...@gmail.com> ha > scritto: > > > > > > > > > > > Well done, Enrico! > > > > > > > > > > > > It seems the CI job seems to be ready to get in. Can you make the > > > > Jenkins > > > > > > job as part of .test-infra? > > > > > > > > > > > > > > > > No, it s not working well. > > > > > I am working on a new maven module which will aggregate the reports > > of > > > > all > > > > > upstream projects but actually it is failing. I have some idea but > > not > > > > much > > > > > time, I am moving on but it will time some other iteration > > > > > > > > > > Enrico > > > > > > > > > > > > > > > > - Sijie > > > > > > > > > > > > On Thu, Feb 15, 2018 at 5:55 AM, Enrico Olivelli < > > > eolive...@gmail.com> > > > > > > wrote: > > > > > > > > > > > > > I have to clean up the patch but actually code coverage report > > > makes > > > > > > sense > > > > > > > and it is reporting a 72% code coverage (using coveralls.io > > KPI). > > > > > > > Most cases of non covered code are about: > > > > > > > - classes which do not have test cases in the same module (I am > > > > working > > > > > > on > > > > > > > this) > > > > > > > - classes which are not abstact but contains only constants or > > > > utility > > > > > > > methods > > > > > > > - interfaces, default methods > > > > > > > - Circe and NativeIO > > > > > > > > > > > > > > Please note that generated code like protobuf, nar or lombok is > > > > already > > > > > > > excluded in the current WIP patch. > > > > > > > > > > > > > > See https://coveralls.io/builds/15432041 > > > > > > > > > > > > > > I think that with little work we can fill most of the gaps, at > > > least > > > > > > > cleaning up the noise. > > > > > > > > > > > > > > I need to finish the work about classes not tested in the same > > > > package > > > > > > then > > > > > > > we will be able to draw a roadmap, creating subtask for missing > > > test > > > > > > cases, > > > > > > > cleaning up interfaces..... > > > > > > > > > > > > > > Enrico > > > > > > > > > > > > > > Il dom 11 feb 2018, 17:43 Enrico Olivelli <eolive...@gmail.com > > > > ha > > > > > > > scritto: > > > > > > > > > > > > > > > Created this job on CI > > > > > > > > > > > > > > > > https://builds.apache.org/job/bookkeeper-code-coverage-wip/ > > > > > > > > > > > > > > > > I am working on a way to create a better report, using this > > > > > suggestion > > > > > > > > > > > > > > > > http://www.lorenzobettini.it/2017/02/jacoco-code-coverage- > > > > > > > and-report-of-multiple-eclipse-plug-in-projects/ > > > > > > > > > > > > > > > > Build takes really long time with JaCoCo instrumentation, so > I > > > will > > > > > use > > > > > > > > Apache CI > > > > > > > > > > > > > > > > Enrico > > > > > > > > > > > > > > > > > > > > > > > > 2018-02-07 17:24 GMT+01:00 Enrico Olivelli < > > eolive...@gmail.com > > > >: > > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > >> 2018-02-05 22:33 GMT+01:00 Sijie Guo <guosi...@gmail.com>: > > > > > > > >> > > > > > > > >>> On Mon, Feb 5, 2018 at 1:04 PM, Enrico Olivelli < > > > > > eolive...@gmail.com > > > > > > > > > > > > > > >>> wrote: > > > > > > > >>> > > > > > > > >>> > Il lun 5 feb 2018, 18:11 David Rusek <d...@streaml.io> > ha > > > > > scritto: > > > > > > > >>> > > > > > > > > >>> > > It sounds like we didn't do anything with the info for > a > > > long > > > > > > time. > > > > > > > >>> > Enrico, > > > > > > > >>> > > I'm glad you're looking at it! Are you planning on > filing > > > > some > > > > > > > issues > > > > > > > >>> > > related to interpreting the coverage data and improving > > it? > > > > > > > >>> > > > > > > > > > >>> > > > > > > > > >>> > > > > > > > > >>> > It was long time ago when I started to experiment with > > > > bookkeeper > > > > > > > >>> codebase. > > > > > > > >>> > We had some problems and I had other priorities. > > > > > > > >>> > I will try to resume this thread on next weeks I think > that > > > the > > > > > > > >>> culprit of > > > > > > > >>> > our problems was the way we were performing BC tests. > > > > > > > >>> > I have not much time so I will go on one step at a time, > if > > > you > > > > > > have > > > > > > > >>> time > > > > > > > >>> > any help is appreciated. > > > > > > > >>> > > > > > > > > >>> > First step will be to test locally jacoco and then to > > restore > > > > the > > > > > > CI > > > > > > > >>> jobs > > > > > > > >>> > > > > > > > > >>> > > > > > > > >>> just one suggestion when you are trying to restore CI jobs, > > > > please > > > > > > > start > > > > > > > >>> with a separate CI job and let the CI job run for a while > to > > > > ensure > > > > > > it > > > > > > > >>> doesn't have any side efforts before enforcing it on the > > other > > > > > jobs. > > > > > > > >>> > > > > > > > >>> > > > > > > > >> > > > > > > > >> Create a new PR to upgrade Code Coverage configuration > > > > > > > >> https://github.com/apache/bookkeeper/pull/1129 > > > > > > > >> > > > > > > > >> This is an example of current master report: > > > > > > > >> https://coveralls.io/jobs/33538314 > > > > > > > >> > > > > > > > >> we are at 61 % (using default metrics) > > > > > > > >> > > > > > > > >> Enrico > > > > > > > >> > > > > > > > >> > > > > > > > >>> > > > > > > > >>> > > > > > > > > >>> > Ideally I would like to have some automated way to keep > an > > > eye > > > > on > > > > > > BK > > > > > > > >>> and > > > > > > > >>> > maybe (not sure it is a big deal) to perform code > coverage > > > > > analysis > > > > > > > >>> even on > > > > > > > >>> > PRs. > > > > > > > >>> > > > > > > > > >>> > One big problem is that our corpus of tests is very heavy > > as > > > > most > > > > > > of > > > > > > > >>> the > > > > > > > >>> > tests start a new cluster. > > > > > > > >>> > Recently we started to use mockito in order to perform > > > narrower > > > > > > unit > > > > > > > >>> > testing. > > > > > > > >>> > > > > > > > > >>> > Stay tuned > > > > > > > >>> > > > > > > > > >>> > Enrico > > > > > > > >>> > > > > > > > > >>> > Enrico > > > > > > > >>> > > > > > > > > >>> > > > > > > > > > >>> > > -Dave > > > > > > > >>> > > > > > > > >> > > > > > > > >> > > > > > > > > -- > > > > > > > > > > > > > > > > > > > > > -- Enrico Olivelli > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > > > > -- Enrico Olivelli > > > > > > > > > > > > > > > -- > > > -- Enrico Olivelli >
