Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct way to not report coverage of individual parallel jobs #1513

Open
efaulhaber opened this issue Dec 30, 2020 · 16 comments
Open

Correct way to not report coverage of individual parallel jobs #1513

efaulhaber opened this issue Dec 30, 2020 · 16 comments

Comments

@efaulhaber
Copy link

We don't want to make individual parallel jobs show as failed if the total coverage didn't decrease.
As advised in #1505, we removed the flag-name (as opposed to the docs of the action, where it's marked as mandatory for parallel runs).
We changed it in this PR, and it worked as expected in this PR:
grafik

However, after merging into master, the runs only report one job each which greatly decreases our total coverage. With the exact same workflow, we got the same terrible coverage on all runs since then.

So, what is the correct way to achieve this? Is this the correct way, even though it's against the advice in the docs? If so, why doesn't it work on master for us?

@afinetooth
Copy link
Collaborator

hi @erik-f,

I see the issue and it was my mistake to offer the workaround to remove flag-name. I was personally unaware it's required for GH Actions CI, as it's not for parallel builds on other CIs, but it is clearly documented there.

Please let me see if I can get you another solution.

In the meantime, the reason you're only getting one job per build after merging into master is that, apparently only in that context, without the flag-name parameter, the API can't distinguish between each incoming parallel job by another ID (such as service_job_id per our API Reference). This leads the API to think the job is a duplicate and delete the last job in favor of the new one, resulting in only the last job being left.

I'll run some tests to see if there's a workaround and get back asap.

sloede added a commit to trixi-framework/Trixi.jl that referenced this issue Dec 31, 2020
It is required as per the [docs][1] and was again confirmed to be
necessary in a GitHub [issue][2] created by @erik-f.

[1]: https://github.com/marketplace/actions/coveralls-github-action#inputs
[2]: lemurheavy/coveralls-public#1513 (comment)
@afinetooth
Copy link
Collaborator

afinetooth commented Jan 1, 2021

@erik-f I ran a number of tests and unfortunately I can't find a workaround for you right now.

I submitted some suggestions to our dev team that, given their expertise might yield such an opportunity after code changes, but, for now, I'm afraid flag-name will be required for parallel jobs using the Coveralls Github Action.

Assuming the issue you were originally trying to workaround was avoiding check failures when these settings apply:

  • COVERAGE THRESHOLD FOR FAILURE
  • COVERAGE DECREASE THRESHOLD FOR FAILURE

Have you considered setting those to values that result in them practically never firing?

In this case, while you'll still see all checks, the chance of one failure blocking a merge (when total coverage doesn't change) should be insignificant.

You have a wide range of values to use, but I believe these give you the widest margin:

Screen Shot 2021-01-01 at 1 11 55 PM

On a hunch, I made a feature change request. Can you verify you'd like to see those coverage threshold checks only apply to the overall check for total coverage? (Or at least to be able to configure as such?)

@efaulhaber
Copy link
Author

Thank you for looking into it. We don't block PRs if coverage checks are failing, so it's more of an aesthetic problem.
Yes, the feature I'd like to have would be an option to disable coverage threshold checks for individual jobs. Additional to this, an option to not show individual reports at all in PRs (like in the PR I linked above without flag-name) would also be a nice feature.

@afinetooth
Copy link
Collaborator

Ok, thanks for confirming.

FYI, I’ve submitted two feature change requests:

  1. Only apply coverage threshold checks to overall coverage
  2. Make the use of flag-name optional for parallel builds using the Coveralls GitHub Action.

@sloede
Copy link

sloede commented Jan 5, 2021

Thanks @afinetooth for your fast feedback and helpful response! It's great to see that this will be worked on, since I assume that other projects will benefit from this as well.

If it is not too much to ask, can you give us a rough estimate (ballpark time frame) when you expect either of these features to become available?

@afinetooth
Copy link
Collaborator

Thanks @sloede . I'm afraid I can't say. It has to go through a PM and be evaluated, scheduled, etc. As soon as I know, I'll feed back here. Feel free to check in by email to support@coveralls.io (just reference this issue).

@mobilutz
Copy link

@sloede @afinetooth We are currently also having the exact same problem, that some parallel coverage runs turn red because they are running a different set of files than the run from the target branch. BUT the overall coverage most often increased.

Just like @sloede said, this is not stopping us from merging. But as was stated as well, it is an not well situation seeing some of the action runs red 😦

@afinetooth
Copy link
Collaborator

Thanks @mobilutz. I plus-one'd both feature requests with your comments.

@lekemula
Copy link

@sloede @afinetooth We are currently also having the exact same problem, that some parallel coverage runs turn red because they are running a different set of files than the run from the target branch. BUT the overall coverage most often increased.

Just like @sloede said, this is not stopping us from merging. But as was stated as well, it is an not well situation seeing some of the action runs red 😦

@sloede @afinetooth The workaround that we have found to this has been making flag-name unique by using github.run_id (but technically could be any random generated number)

-        flag-name: run-${{ matrix.ci_node_index }}
+        flag-name: run-${{ matrix.ci_node_index }}-${{ github.run_id }}

image

It's worth to note however that we don't consider this solution ideal because it leads to bloating of github checks:

image

@casperdcl
Copy link

yup this issue is a duplicate of the incorrectly closed #1505.

@jrfnl
Copy link

jrfnl commented May 13, 2021

FYI, I’ve submitted two feature change requests:

  • Only apply coverage threshold checks to overall coverage
  • Make the use of flag-name optional for parallel builds using the Coveralls GitHub Action.

@afinetooth Any chance of giving us an update on the current status ?

@afinetooth
Copy link
Collaborator

afinetooth commented May 14, 2021

Hi @jrfnl, I don't have an update besides still in backlog, as opposed to sprint log. We're an extremely small team. I understand this is affecting a decent number of folk though and since it's breaking some of your PR workflows I will do my best to raise the priority and obtain some kind of ETA.

@jrfnl
Copy link

jrfnl commented May 15, 2021

I will do my best to raise the priority and obtain some kind of ETA. Please stand by.

Much appreciated. Standing by with bated breath...

@senid231
Copy link

we still have very same issue

@senid231
Copy link

@afinetooth any news about the fix?

@senid231
Copy link

senid231 commented Sep 14, 2022

I've tried a workaround @lekemula suggested but no luck - I see -28% coverage drop for PR that has only config file change

yeti-switch/yeti-web#1176

https://coveralls.io/builds/52464915

Coverage decreased (-28.6%) to 55.06%
changes - 1 line removed, 3 lines added

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants