Skip to Content
  2. Mailing Lists
  3. Contributors
  4. 100 % coverage not enough?

Archives

Contributors

Login Case Sensative

A (hope not stupid) question about wire transfer payment transactions

100 % coverage not enough? 

Hi all,


I submitted a PR with 100% code coverage to the account-invoicing repo.


However the branch is red because 100% is apparently not enough. 
Supposedly the PR would decrease the overall coverage percentage. How??


See here: https://github.com/OCA/account-invoicing/pull/1544


I think any PR that gets code coverage > 90% (and that does not decrease 
coverage of an existing module), should get a branch green, but 
certainly 100%.


Kind regards, Ronald
by "Ronald Portier" <rportier@therp.nl> - 08:56 - 8 Sep 2023

Follow-Ups

  • Re: 100 % coverage not enough?
    El lun, 11-09-2023 a las 08:41 +0000, Ronald Portier escribió:
    The decrease must be an artifact of comparing a branch based on an older version of the main branch, that had less coverage overall, with the current version of the main branch

    Maybe coverage can be compared to the latest parent commit of the oldest PR commit? That way, we could be sure if it's really increasing.
    by Jairo Llopis - 08:40 - 13 Sep 2023
  • Re: 100 % coverage not enough?

    Hi,


    Rebasing the branch, as suggegested by Jairo solved the problem. So let us keep that in mind with future occurences of this issue.


    Denis, I understand why you do not want to remove the check, but I do not see how a PR that only concerns one module, could influence other modules. The decrease must be an artifact of comparing a branch based on an older version of the main branch, that had less coverage overall, with the current version of the main branch. However merging such a PR will not undo the changes that have occurred in the main branch since creating the PR/feature branch. So the proper measure on whether a PR increases or decreases overall coverage would be to compare the coverage percentage in only the patch (the changed code) with the overall coverage of the project (the main branch).


    Kind regards, Ronald


    Op 11-09-2023 om 10:22 schreef Roussel, Denis:
    The answer is in the details link.

    @jairo I don't agree about removing that check.

    The 'patch' coverage report concerns exclusively the code you introduce with your PR.
    The 'project' coverage report concerns the already existing code outside your PR. So, a decrease there means that your PR influences other modules (not executing a super in an inherited method, ...)

    Even if coverage is not mandatory in OCA, it gives indicators on technical tests quality.

    My two cents

    On Mon, Sep 11, 2023 at 10:11 AM jairo <notifications@odoo-community.org> wrote:
    El vie, 08-09-2023 a las 18:57 +0000, Ronald Portier escribió:
    Supposedly the PR would decrease the overall coverage percentage. How??

    The 1st thing you need to know is that hose checks are not required.

    Now, this is how your PR looks:


    That ✅ is telling the reviewer that the PR has a 100% of coverage. That's the important part.

    The ❌ however is the weirdo. Sometimes it happens, so I systematically ignore that check. IMHO it could be removed. My guess is that the base branch evolved and got more coverage, and that if you rebased it would always increase. But maybe it's just a bug somewhere.

    _______________________________________________
    Mailing-List: https://odoo-community.org/groups/contributors-15
    Post to: mailto:contributors@odoo-community.org
    Unsubscribe: https://odoo-community.org/groups?unsubscribe



    --

    Denis Roussel
    Software Engineer
    T    : +32 2 888 31 49
    M : +32 472 22 00 57

    Val Benoit, Quai Banning 6 | B-4000 Liège | Belgium
    Atrium Building, Drève Richelle 167 | B-1410 Waterloo | Belgium
    Zone industrielle 22 | L-8287 Kehlen | Luxembourg

    _______________________________________________
    Mailing-List: https://odoo-community.org/groups/contributors-15
    Post to: mailto:contributors@odoo-community.org
    Unsubscribe: https://odoo-community.org/groups?unsubscribe

    by "Ronald Portier" <rportier@therp.nl> - 10:40 - 11 Sep 2023
  • Re: 100 % coverage not enough?
    The answer is in the details link.

    @jairo I don't agree about removing that check.

    The 'patch' coverage report concerns exclusively the code you introduce with your PR.
    The 'project' coverage report concerns the already existing code outside your PR. So, a decrease there means that your PR influences other modules (not executing a super in an inherited method, ...)

    Even if coverage is not mandatory in OCA, it gives indicators on technical tests quality.

    My two cents

    On Mon, Sep 11, 2023 at 10:11 AM jairo <notifications@odoo-community.org> wrote:
    El vie, 08-09-2023 a las 18:57 +0000, Ronald Portier escribió:
    Supposedly the PR would decrease the overall coverage percentage. How??

    The 1st thing you need to know is that hose checks are not required.

    Now, this is how your PR looks:


    That ✅ is telling the reviewer that the PR has a 100% of coverage. That's the important part.

    The ❌ however is the weirdo. Sometimes it happens, so I systematically ignore that check. IMHO it could be removed. My guess is that the base branch evolved and got more coverage, and that if you rebased it would always increase. But maybe it's just a bug somewhere.

    _______________________________________________
    Mailing-List: https://odoo-community.org/groups/contributors-15
    Post to: mailto:contributors@odoo-community.org
    Unsubscribe: https://odoo-community.org/groups?unsubscribe



    --

    Denis Roussel
    Software Engineer
    T    : +32 2 888 31 49
    M : +32 472 22 00 57


    Val Benoit, Quai Banning 6 | B-4000 Liège | Belgium
    Atrium Building, Drève Richelle 167 | B-1410 Waterloo | Belgium
    Zone industrielle 22 | L-8287 Kehlen | Luxembourg
    by Denis Roussel - 10:21 - 11 Sep 2023
  • Re: 100 % coverage not enough?
    El vie, 08-09-2023 a las 18:57 +0000, Ronald Portier escribió:
    Supposedly the PR would decrease the overall coverage percentage. How??

    The 1st thing you need to know is that hose checks are not required.

    Now, this is how your PR looks:


    That ✅ is telling the reviewer that the PR has a 100% of coverage. That's the important part.

    The ❌ however is the weirdo. Sometimes it happens, so I systematically ignore that check. IMHO it could be removed. My guess is that the base branch evolved and got more coverage, and that if you rebased it would always increase. But maybe it's just a bug somewhere.
    by Jairo Llopis - 10:11 - 11 Sep 2023

    1 attachments 1 attachments

    Imagen-VDE3A2.png
  • Re: 100 % coverage not enough?
    Hi Ronald,

    I replied on the PR.

    Bests

    On Fri, Sep 8, 2023 at 8:57 PM Ronald Portier <notifications@odoo-community.org> wrote:
    Hi all,


I submitted a PR with 100% code coverage to the account-invoicing repo.


However the branch is red because 100% is apparently not enough. 
Supposedly the PR would decrease the overall coverage percentage. How??


See here: https://github.com/OCA/account-invoicing/pull/1544


I think any PR that gets code coverage > 90% (and that does not decrease 
coverage of an existing module), should get a branch green, but 
certainly 100%.


Kind regards, Ronald

    _______________________________________________
    Mailing-List: https://odoo-community.org/groups/contributors-15
    Post to: mailto:contributors@odoo-community.org
    Unsubscribe: https://odoo-community.org/groups?unsubscribe



    --
    Simone Orsi

    Full stack Python web developer, Odoo specialist, Odoo Community Board Member, in love with open source.
    by Simone Orsi - 07:55 - 11 Sep 2023