Archives
Contributors
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
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
--
Val Benoit, Quai Banning 6 | B-4000 Liège | Belgium
Atrium Building, Drève Richelle 167 | B-1410 Waterloo | BelgiumZone 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
Reference
-
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-
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 branchMaybe 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
--
Val Benoit, Quai Banning 6 | B-4000 Liège | Belgium
Atrium Building, Drève Richelle 167 | B-1410 Waterloo | BelgiumZone 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 centsOn 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
--
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 -
Re: 100 % coverage not enough?
Hi Ronald,I replied on the PR.BestsOn 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 OrsiFull stack Python web developer, Odoo specialist, Odoo Community Board Member, in love with open source.
by Simone Orsi - 07:55 - 11 Sep 2023
-
