I followed Udi's suggestion for disabling the annotations ( https://github.com/apache/beam/pull/12944) and filed a JIRA to revert it ( https://issues.apache.org/jira/browse/BEAM-10977) once the issue is resolved.
On Thu, Sep 24, 2020 at 11:15 AM Robert Bradshaw <rober...@google.com> wrote: > I have found the code coverage to be useful when it pertains to the diff > in question, but often it seems to be based on something else (which is > also where it gets really noisy). Perhaps it would make sense to hide them > by default until this is resolved. > > On Thu, Sep 24, 2020 at 11:11 AM Udi Meiri <eh...@google.com> wrote: > >> I was hoping to use code coverage as a tool in reviews, to tell me if an >> important or significant amount of code is not tested. >> As for reviewer requirements, we should probably discuss that here. >> >> On Thu, Sep 24, 2020 at 10:25 AM Ahmet Altay <al...@google.com> wrote: >> >>> >>> >>> On Wed, Sep 23, 2020 at 5:49 PM Udi Meiri <eh...@google.com> wrote: >>> >>>> You can hide annotations in the UI when looking at the diff (under the >>>> ... menu). >>>> >>>> https://docs.codecov.io/docs/github-checks-beta#hiding-annotations-in-the-files-view >>>> >>> >>> Thank you. If it is just me, and if this is a one time thing I am happy >>> to do this. This raises a question, if we are allowed to hide annotations, >>> can we also ignore their messages during a review? >>> >>> >>>> >>>> >>>> There is an option to disable annotations (I'm not opposed to it): >>>> https://docs.codecov.io/docs/codecovyml-reference#github_checks-github-users-only >>>> >>> >>> What do others think? Can we improve codecov settings to be more >>> specific as an alternative? How much room we have for improvement? >>> >>> >>>> >>>> >>>> On Wed, Sep 23, 2020 at 3:07 PM Ahmet Altay <al...@google.com> wrote: >>>> >>>>> Hi all, >>>>> >>>>> Thank you for adding this. >>>>> >>>>> I have a few questions. codecov reports are crowding the PR reviews >>>>> quite a bit. Is it possible that there is a misconfiguration? Could we >>>>> limit the annotations to files in review? Would it make sense/possible to >>>>> disable the annotations by default? >>>>> >>>>> /cc @Yifan Mai <yifan...@google.com> - had similar questions. >>>>> >>>>> Thank you, >>>>> Ahmet >>>>> >>>>> >>>>> On Mon, Jul 6, 2020 at 4:08 PM Udi Meiri <eh...@google.com> wrote: >>>>> >>>>>> The idea was to add coverage to one of the already run precommit tox >>>>>> tasks. This should keep the additional overhead to a minimum. >>>>>> py37-cloud is the current candidate, since it contains the most >>>>>> dependencies so fewer tests are skipped. >>>>>> Saavan, do you have an estimate for the overhead? >>>>>> >>>>>> Once coverage information is available, we could make use of it for >>>>>> review purposes. >>>>>> >>>>>> >>>>>> On Mon, Jul 6, 2020 at 3:06 PM Mikhail Gryzykhin < >>>>>> gryzykhin.mikh...@gmail.com> wrote: >>>>>> >>>>>>> I wouldn't consider build time as a blocker to add report. Even if >>>>>>> build time is rather slower, we can run coverage report periodically as >>>>>>> a >>>>>>> separate job and still get use of it. >>>>>>> >>>>>>> On Mon, Jul 6, 2020, 2:38 PM Robert Bradshaw <rober...@google.com> >>>>>>> wrote: >>>>>>> >>>>>>>> This sounds useful to me, and as it's purely informational would be >>>>>>>> a >>>>>>>> low cost to try out. The one question is how it would impact build >>>>>>>> runtimes--do you have an estimate for what the cost is here? >>>>>>>> >>>>>>>> On Sun, Jul 5, 2020 at 1:14 PM Saavan Nanavati <saa...@google.com> >>>>>>>> wrote: >>>>>>>> > >>>>>>>> > Hey everyone, >>>>>>>> > >>>>>>>> > Currently, during the Jenkins build process, we don't generate >>>>>>>> any code coverage reports for the Python SDK. This email includes a >>>>>>>> proposal to generate python coverage reports during the pre-commit >>>>>>>> build, >>>>>>>> upload them to codecov.io for analysis and visualization, and >>>>>>>> automatically post the resulting stats back to GitHub PRs to help >>>>>>>> developers decide whether their tests need revision. >>>>>>>> > >>>>>>>> > You can view/comment on the proposal here, or the full text of >>>>>>>> the proposal at the end of this email. Let me know what you think, or >>>>>>>> if >>>>>>>> there are any suggestions for improvements. Thanks! >>>>>>>> > >>>>>>>> > Python Coverage Reports For CI/CD >>>>>>>> > >>>>>>>> > Author: Saavan Nanavati (saa...@google.com) >>>>>>>> > >>>>>>>> > Reviewer: Udi Meiri (eh...@google.com) >>>>>>>> > >>>>>>>> > >>>>>>>> > Overview >>>>>>>> > >>>>>>>> > >>>>>>>> > This is a proposal for generating code coverage reports for the >>>>>>>> Python SDK during Jenkins’ pre-commit phase, and uploading them to >>>>>>>> codecov.io for analysis, with integration back into GitHub using >>>>>>>> the service’s sister app. >>>>>>>> > >>>>>>>> > >>>>>>>> > This would extend the pre-commit build time but provide valuable >>>>>>>> information for developers to revise and improve their tests before >>>>>>>> their >>>>>>>> PR is merged, rather than after when it’s less likely developers will >>>>>>>> go >>>>>>>> back to improve their coverage numbers. >>>>>>>> > >>>>>>>> > >>>>>>>> > This particular 3rd party service has a litany of awesome >>>>>>>> benefits: >>>>>>>> > >>>>>>>> > It’s free for open-source projects >>>>>>>> > >>>>>>>> > It seamlessly integrates into GitHub via a comment-bot (example >>>>>>>> here) >>>>>>>> > >>>>>>>> > It overlays coverage report information directly onto GitHub code >>>>>>>> using Sourcegraph >>>>>>>> > >>>>>>>> > It requires no changes to Jenkins, thereby reducing the risk of >>>>>>>> breaking the live test-infra >>>>>>>> > >>>>>>>> > It’s extensible and can later be used for the Java & Go SDKs if >>>>>>>> it proves to be awesome >>>>>>>> > >>>>>>>> > It has an extremely responsive support team that’s happy to help >>>>>>>> open-source projects >>>>>>>> > >>>>>>>> > >>>>>>>> > A proof-of-concept can be seen here and here. >>>>>>>> > >>>>>>>> > >>>>>>>> > Goals >>>>>>>> > >>>>>>>> > >>>>>>>> > Provide coverage stats for the Python SDK that update with every >>>>>>>> pre-commit run >>>>>>>> > >>>>>>>> > Integrate these reports into GitHub so developers can take >>>>>>>> advantage of the information >>>>>>>> > >>>>>>>> > Open a discussion for how these coverage results can be utilized >>>>>>>> in code reviews >>>>>>>> > >>>>>>>> > >>>>>>>> > Non-Goals >>>>>>>> > >>>>>>>> > Calculate coverage statistics using external tests located >>>>>>>> outside of the Python SDK >>>>>>>> > >>>>>>>> > >>>>>>>> > This is ideal, but would require not only merging multiple >>>>>>>> coverage reports together but, more importantly, waiting for these >>>>>>>> tests to >>>>>>>> be triggered in the first place. The main advantage of calculating >>>>>>>> coverage >>>>>>>> during pre-commit is that developers can revise their PRs before >>>>>>>> merging, >>>>>>>> which is not guaranteed if this is a goal. >>>>>>>> > >>>>>>>> > >>>>>>>> > However, it could be something to explore for the future. >>>>>>>> > >>>>>>>> > Background >>>>>>>> > >>>>>>>> > >>>>>>>> > Providing code coverage for the Python SDK has been a problem >>>>>>>> since at least 2017 (BEAM-2762) with the previous solution being to >>>>>>>> calculate coverage in post-commit with coverage.py, and then sending >>>>>>>> the >>>>>>>> report to coveralls.io which would post to GitHub. At some point, >>>>>>>> this solution broke and the Tox environment used to compute coverage, >>>>>>>> cover, was turned off but still remains in the codebase. >>>>>>>> > >>>>>>>> > >>>>>>>> > There have been 4 main barriers, in the past, to re-implementing >>>>>>>> coverage that will be addressed here. >>>>>>>> > >>>>>>>> > >>>>>>>> > It’s difficult to unify coverage for some integration tests, >>>>>>>> especially ones that rely on 3rd party dependencies like GCP since >>>>>>>> it’s not >>>>>>>> possible to calculate coverage for the dependencies. >>>>>>>> > >>>>>>>> > As stated earlier, this is a non-goal for the proposal. >>>>>>>> > >>>>>>>> > >>>>>>>> > The test reporter outputs results in the same directory which >>>>>>>> sometimes causes previous results to be overwritten. This occurs when >>>>>>>> using >>>>>>>> different parameters for the same test (e.g. running a test with >>>>>>>> Dataflow >>>>>>>> vs DirectRunner). >>>>>>>> > >>>>>>>> > This was mainly a post-commit problem but it does require >>>>>>>> exploration since it could be an issue for pre-commit. However, even >>>>>>>> in the >>>>>>>> worst case, the coverage numbers would still be valuable since you can >>>>>>>> still see how coverage changed relatively between commits even if the >>>>>>>> absolute numbers are slightly inaccurate. >>>>>>>> > >>>>>>>> > >>>>>>>> > It’s time-consuming and non-trivial to modify and test changes to >>>>>>>> Jenkins >>>>>>>> > >>>>>>>> > We don’t need to - this proposal integrates directly with >>>>>>>> codecov.io, making Jenkins an irrelevant part of the testing >>>>>>>> infrastructure with regards to code coverage - it’s not just easier, >>>>>>>> it’s >>>>>>>> better because it provides many benefits (mentioned in the Overview >>>>>>>> section) that a Jenkins-based solution like Cobertura doesn’t have. >>>>>>>> Lastly, >>>>>>>> using codecov.io would place less strain and maintenance on >>>>>>>> Jenkins (less jobs to run, no need for storing the reports, etc.). >>>>>>>> > >>>>>>>> > >>>>>>>> > coveralls.io isn’t the best to work with (email thread) >>>>>>>> > >>>>>>>> > codecov.io ;) >>>>>>>> > >>>>>>>> > Design Details >>>>>>>> > >>>>>>>> > >>>>>>>> > To utilize codecov.io, the existing Tox environment cover needs >>>>>>>> to be added to the list of toxTasks that Gradle will run in >>>>>>>> pre-commit. To >>>>>>>> reduce strain (and more-or-less duplicate information), coverage only >>>>>>>> needs >>>>>>>> to be calculated for one Python version (say 3.7, with cloud >>>>>>>> dependencies) >>>>>>>> rather than all of them. >>>>>>>> > >>>>>>>> > >>>>>>>> > To be compatible with Jenkins and codecov.io, the Tox >>>>>>>> environment should be configured as follows: >>>>>>>> > >>>>>>>> > passenv = GIT_* BUILD_* ghprb* CHANGE_ID BRANCH_NAME JENKINS_* >>>>>>>> CODECOV_* >>>>>>>> > >>>>>>>> > deps = >>>>>>>> > >>>>>>>> > ... >>>>>>>> > >>>>>>>> > codecov >>>>>>>> > >>>>>>>> > commands = >>>>>>>> > >>>>>>>> > ... >>>>>>>> > >>>>>>>> > codecov -t CODECOV_TOKEN >>>>>>>> > >>>>>>>> > >>>>>>>> > There are two options for what goes into the ellipsis (...) >>>>>>>> First, we can use coverage.py to compute coverage (which is how it’s >>>>>>>> currently set up) but that uses the old nose test runner. >>>>>>>> Alternatively, we >>>>>>>> can switch to computing coverage with pytest and pytest-cov, which >>>>>>>> would >>>>>>>> give more accurate numbers, and is the preferred solution. >>>>>>>> > >>>>>>>> > >>>>>>>> > Additionally, the CODECOV_TOKEN, which can be retrieved here, >>>>>>>> should be added as an environment variable in Jenkins. >>>>>>>> > >>>>>>>> > >>>>>>>> > Finally, a YAML file should be added so codecov.io can resolve >>>>>>>> file path errors. This can also be used to define success thresholds >>>>>>>> and >>>>>>>> more (for example, here) though, ideally, this coverage task should >>>>>>>> never >>>>>>>> fail a Jenkins build due to the risk of false-negatives. >>>>>>>> > >>>>>>>> > >>>>>>>> > A proof-of-concept for this design can be seen here and here >>>>>>>> (this is code coverage for my fork). >>>>>>>> > >>>>>>>> > >>>>>>>> >>>>>>>
