[10159] Fix codecov.io reporting. Move codecov.io / coveralls reporting outside of tox environments. #1574

adiroiban · 2021-03-28T13:31:30Z

Scope and purpose

Reporting coverage to codecov.io and coveralls should only be needed for CI.

For CI we already have python virtual env created.

That venv can be reused for codecov/coveralls operations so that we don't need to add tox extra complexity for those calls.

It should also make the tox.ini cleaner and move the coverage publish code closer to where it's used.

Coverage reporting also broken with codecov reports for windows linux and mac not merged.
Coveralls reports are not normalized.

Changes

There is a problem with codecov.io reporting as the coverage is reported for the PR auto-merge commit.
I saw this error for GitHub Coverage and Azure Coverage.

The error is

 Issue detecting commit SHA. Please run actions/checkout with fetch-depth > 1 or set to 0

fetch-depth: 2 was enabled for codecov bash uploaded.

Generating XML+ codecov.io publish was taking 40 seconds with the most time taken by XML generation.
Only coveralls publish was also taking 40 seconds.

If coveralls is not used by Twisted devs we can stop publishing it and save 40 seconds with each build.

With coverage publishing handling moved outside of tox.ini there is no speed improvement.
I still prefer the coverage publishing outside of tox as I find it easier to understand how things works.

The coverage config was updated to normalize the path and ignore the trial temp file.

The CI steps were updated to show the status of the local coverage... to help debugging.

Any errors generated during coverage reporting are ignored.
The idea is to reduce random failures.
Once we have the other CI jobs with a solid green we can look at requiring strict rules.

Contributor Checklist:

The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/10159
[NO] I ran tox -e lint to format my patch to meet the Twisted Coding Standard
I have created a newsfragment in src/twisted/newsfragments/ (see: News files)
The title of the PR starts with the associated Trac ticket number (without the # character).
[NA] I have updated the automated tests and checked that all checks for the PR are green.
I have submitted the associated Trac ticket for review by adding the word review to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.
The merge commit will use the below format
The first line is automatically generated by GitHub based on PR ID and branch name.
The other lines generated by GitHub should be replaced.

Merge pull request #123 from twisted/4356-branch-name-with-trac-id

Author: adiroiban
Reviewer: 
Fixes: ticket:10159

Fix codecov reporting for a PR. Move coverage CI reporting outside of tox.

adiroiban · 2021-03-28T14:35:46Z

The builds are now reported for both GHA and Azure https://codecov.io/gh/twisted/twisted/commit/4794ef2b15244904655b4c9dd20eae352fe88ae4/build

…on macOS.

adiroiban · 2021-03-28T14:58:28Z

.coveralls.yml

@@ -1 +0,0 @@
-repo_token: "JFDTIRUVOQ8jCM3zcajrZALlpKXyiXGAX"


This should be private.
For now, I have moved it inside each CI config. In this way we don't need to install pyyaml just to read this line.

If it were 'private' it wouldn't be available to PRs from forks so we wouldn't get coverage on them. (for some definition of private)

altendky

If it doesn't speed anything up then I'm not clear how it is better to repeat the (little bit of) code in two CI configurations than to just have it in tox.ini. Though sure, it doesn't presently seem to be actually the same between the two since GHA uses the Python uploader and AP uses the bash uploader. :[

There are some minor changes requested, but at this point mostly I'm interested in some discussion.

.github/workflows/test.yaml

altendky · 2021-03-28T19:34:33Z

.github/workflows/test.yaml

+
+    - name: Publish coveralls
+      # We want to publish coverage even on failure.
+      if: contains(matrix['tox-env'], 'withcov') || failure()


Won't this publish on failure even if withcov isn't in the tox env? I do see this already existed a few lines up.

true. Thanks!

My point was that this should only publish if withcov was specified. I don't know how to do this in a simple way, and I haven't thought through the not-simple way. So, just always doing it and ignoring any potential errors when there's no coverage data (which apparently there either aren't or they have already been dealt with, what with the passing CI (or we don't exercise this case)) may be the best that's worth doing anyways. And that's what we have now.

I guess the tl;dr is that I think if: always() is 'wrong' but maybe GHA isn't friendly enough for it to be worth making it right.

altendky · 2021-03-28T19:35:37Z

.github/workflows/test.yaml

-      run: tox -e coverage-prepare,codecov-push,coveralls-push
+      run: |
+        python -m coverage xml -o coverage.xml -i
+        python -m codecov -n 'lnx-${{ matrix.python-version }}-${{ matrix.tox-env }}${{ matrix.noipv6 }}' -X search -X gcov -f coverage.xml


My understanding is that Codecov prefers use of the bash uploader.

Thanks for the headup and the review.

I though that I have moved to bash, and this is why I haven't left another comment. My bad.

I will update this to use the bash uploader

from https://github.com/codecov/example-python

Q: What's the difference between the codecov-bash and codecov-python uploader? A: As far as python is concerned, nothing. You may choose to use either uploader. Codecov recommends using the bash uploader when possible as it supports more unique repository setups.

altendky · 2021-03-28T19:39:01Z

azure-pipelines/run_test_steps.yml

+  # A minimum dept of 2 is required, as for PR head is the auto-merge commit
+  # and we need the last commit from PR to report the coverage.


This is very possibly a topic for a separate PR, but what about just skipping the automatic merge commit and checking out the actual branch instead? It's not like the automatic merge commit actually keeps the CI report up to date with the target branch. Relatedly, having dropped the requirement that branches are up to date seems a bit hazardous.

Suggested change

# A minimum dept of 2 is required, as for PR head is the auto-merge commit

# and we need the last commit from PR to report the coverage.

# A minimum depth of 2 is required, as the default is the auto-merge commit

# and we need the last commit from the PR to report the coverage.

What code actually looks back up to the parent to use this? Maybe this relates to the long-standing complaints about sometimes getting wrong delta coverage reports?

Thanks for the typo fixes.

Note that this is an issue with Azure Pipelines and not GitHub actions ... so we need different docs and I don't think there is this option

https://docs.microsoft.com/en-us/azure/devops/pipelines/yaml-schema?view=azure-devops&tabs=schema%2Cparameter-schema#checkout

The codecov bash script in Azure needs this.

Running the test on branch HEAD vs merge-commit should be debated outside of this PR.

Sorry, too much GHA on my mind. Thanks for the correction. I would expect that AP has some variable that could be used to specify the branch instead of the merge commit. But agreed, no need for a debate here.

azure-pipelines/run_test_steps.yml

altendky · 2021-03-28T19:45:16Z

azure-pipelines/run_test_steps.yml

+- bash: |
+    python -m coverage xml -o coverage.xml -i
+    bash <(curl -s https://codecov.io/bash) -X search -X gcov -f coverage.xml -v -n "${{ parameters.platform }}-${{ parameters.pythonVersion }}-alldeps-withcov"
+  displayName: 'Generate XMl and report codecov.io'


Capitalization and may as well match above I guess...

Suggested change

displayName: 'Generate XMl and report codecov.io'

displayName: 'Generate XML and publish to Codecov'

adiroiban · 2021-03-29T13:38:36Z

Thanks @altendky for the review

If it doesn't speed anything up then I'm not clear how it is better to repeat the (little bit of) code in two CI configurations than to just have it in tox.ini. Though sure, it doesn't presently seem to be actually the same between the two since GHA uses the Python uploader and AP uses the bash uploader. ``

On Azure we need special handling for coveralls... and for azure we also need 2 separate tox env -- to fake CircleCI only for Covealls and not for Codecov.

I tried to move the code to tox.ini but I found the result e bigger mess.
Even if the commands are the same the comand line arguments and environment variables are defined in different ways

The CI configuration already complicated enough with job matrix on GHA and templates in Azure, and the env VARS that need to be created inside the CI YAML config and then passed to TOX.
For me, keeping everything inside the CI YAML config is better.

You can give it a try if you want.

[testenv:ci-coverage-gha]
description = Publish the coverage from an GitHub Actions CI job.
skip_install = True
deps =
    codecov
    coveralls
setenv =
    COVERALLS_REPO_TOKEN='JFDTIRUVOQ8jCM3zcajrZALlpKXyiXGAX'

commands =
    # Convert from internal coverage format to universal Cobertura XML.
    python -m coverage xml -o coverage.xml -i
    python -m codecov -X search -X gcov -f coverage.xml $CODECOV_ARGS
    python -m coveralls


[testenv:ci-coverage-azure-covdecov]
description = Publish to codecov from an Azure CI job.
skip_install = True
deps =
    codecov

commands =
    # Convert from internal coverage format to universal Cobertura XML.
    python -m coverage xml -o coverage.xml -i
    python -m codecov -X search -X gcov -f coverage.xml $CODECOV_ARGS


[testenv:ci-coverage-azure-coveralls]
description = Publish to Coveralls from an Azure CI job.
skip_install = True
deps =
    coveralls

setenv =
    COVERALLS_REPO_TOKEN='JFDTIRUVOQ8jCM3zcajrZALlpKXyiXGAX'

commands =
    # Coveralls has no support for Azure so we pretend to be CircleCI.
    # See https://github.com/lemurheavy/coveralls-public/issues/1272
    export CIRCLECI=1
    CIRCLE_BUILD_NUM=$BUILD_BUILDNUMER
    export CIRCLE_BRANCH=$BUILD_SOURCEBRANCH
    python -m coveralls

and bash version from tox

[testenv:ci-coverage-azure-codecov]
description = Publish to Codecov via bash from an Azure CI job.
skip_install = True

allowlist_externals =
    bash
    curl

commands =
    # Convert from internal coverage format to universal Cobertura XML.
    python -m coverage xml -o coverage.xml -i

    curl -s https://codecov.io/bash -o codecov.sh
    bash codecov.sh  -X search -X gcov -f coverage.xml $CODECOV_ARGS

adiroiban · 2021-03-29T14:35:26Z

Codecov project fails due to

https://app.codecov.io/gh/twisted/twisted/compare/1574/changes#D1L6188 - looks like we don't have tests for imap4_utf_7 codec and a previous report didn't report the branch coverage for it
https://app.codecov.io/gh/twisted/twisted/compare/1574/changes#D2L405 - looks like a timeout was not longer triggered. i suspect the timeout was reported from a previous test failure

altendky

Thanks for the detailed response. Everything looks to be addressed except maybe one bit. The use of the the Python vs. bash codecov uploader on GHA. If you did consider that and decline to change, alrighty. :] No need to hold up on that.

I'll trust your call on the coverage issues.

adiroiban · 2021-03-30T11:30:10Z

Thanks @altendky for the review.

It looks like the merge commit is still pushed... but after some time, codecov.io removes it and merges it with the main PR commit.

So I am not sure if depth=2 is required.

But it looks like it takes a lot of time for codecov.io to send to final report... and I think that this is due to pypy reports that are sent that late.

It's funny, as now the GitHub Actions reports are not visible.

so... still WIP. I will ping you over IRC when things are ready for another check

Thanks!

adiroiban · 2021-03-30T12:16:45Z

.github/workflows/test.yaml

@@ -48,13 +47,14 @@ jobs:
          # end to end functional test usage for non-distributed trial runs.
          - python-version: 3.6.7
            tox-env: nodeps-withcov-posix
-            trial-args: ''
+            trial-args: '-j 4'


this is a drive-by fix, as parallel coverage should be enabled here.

It was accidentally disabled thinking that if left empty it will use the default.

adiroiban · 2021-03-30T12:16:58Z

.github/workflows/test.yaml

          # `noipv6` is created to make sure all is OK on an OS which doesn't
          # have IPv6 available.
          # Any supported Python version is OK for this job.
          - python-version: 3.6
            tox-env: alldeps-withcov-posix
            noipv6: -noipv6
+            trial-args: '-j 4'


this is a drive-by fix, as parallel coverage should be enabled here.

It was accidentally disabled thinking that if missing it will use the default.

adiroiban · 2021-03-30T17:43:37Z

I think that this should be merged as it is as it already has enough improvements.

Coveralls fixes

Publish from Azure as a fake Circle-CI https://coveralls.io/builds/38390418
Publish from Github actions https://coveralls.io/builds/38390167
Paths for both Azure and GitHub are now normalized
NOT-DONE - PR coverage not yet merged

Codecov

Coverage merged for both Azure and GitHub to have unified view.

General changes

Coverage publishing from CI moved outside of tox

altendky

I'll say that I'm feeling pretty itchy about all this. But, given the discussion, I feel like that's just the way it is going to be regardless because of the multiple CI systems * the multiple coverage presenters * the myriad oddities. Oh well, it's mutable, let's go for it.

adiroiban · 2021-03-31T09:49:56Z

Thanks for the review... This is not over yet :) Part 2 at #1543

adiroiban added 3 commits March 28, 2021 14:24

Move coverage reporting outside of tox.

5badeba

Remove coveralls token from root.

236ad7a

Move coveralls token to GHA.

4794ef2

Publish coverate in parallel.

cab2860

adiroiban marked this pull request as ready for review March 28, 2021 14:47

adiroiban requested a review from a team March 28, 2021 14:48

Revert parallel coverage publish as there is no speedup and it fails …

0becaed

…on macOS.

adiroiban commented Mar 28, 2021

View reviewed changes

Move coveralls token for GHA.

f3e0b02

altendky reviewed Mar 28, 2021

View reviewed changes

Update after review.

9e56254

Fix more typos.

afd02da

adiroiban changed the title ~~[10159] Move codecov.io / coveralls reporting outside of tox environments.~~ [10159] Fix Azure codecov.io reporting for PR. Move codecov.io / coveralls reporting outside of tox environments. Mar 29, 2021

adiroiban mentioned this pull request Mar 29, 2021

10141: Avoid trying to call cancel() on the timeout if it is None. #1558

Open

8 tasks

altendky approved these changes Mar 30, 2021

View reviewed changes

adiroiban added 3 commits March 30, 2021 11:31

Merge branch 'trunk' into 10159-codecov-pr-fix

9d19ad6

Use bash for codecov.

9568da2

Don't install codecov python package.

ccb8803

adiroiban added 2 commits March 30, 2021 12:31

Don't use depth 2 on Azure as I don't think it's needed.

69741cf

Enable parallel runs for non pypy.

644493d

adiroiban changed the title ~~[10159] Fix Azure codecov.io reporting for PR. Move codecov.io / coveralls reporting outside of tox environments.~~ [10159] Move codecov.io / coveralls reporting outside of tox environments. Mar 30, 2021

adiroiban changed the title ~~[10159] Move codecov.io / coveralls reporting outside of tox environments.~~ [10159] Fix codecov.io reporting. Move codecov.io / coveralls reporting outside of tox environments. Mar 30, 2021

adiroiban added 2 commits March 30, 2021 13:03

Enable fetch depth on both GHA and Azure.

847571f

Report GHA codecov using github actions.

69fe45d

adiroiban commented Mar 30, 2021

View reviewed changes

adiroiban added 5 commits March 30, 2021 15:32

Don't fail the job on coverage reporting errors.

149b3d0

Try path normalization inside coverage.

e32881a

Move combine outside of main env run.

33f3075

Debug coveralls. Ignore temp.

65641ac

Switch back to emulating circle-ci.

2712310

altendky approved these changes Mar 30, 2021

View reviewed changes

adiroiban merged commit 022659c into trunk Mar 31, 2021

adiroiban deleted the 10159-codecov-pr-fix branch March 31, 2021 00:01

adiroiban mentioned this pull request Mar 31, 2021

[10121] Fix random PyPy CI failures. Improve GHA CI speed. Improve coverage reporting. #1543

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[10159] Fix codecov.io reporting. Move codecov.io / coveralls reporting outside of tox environments. #1574

[10159] Fix codecov.io reporting. Move codecov.io / coveralls reporting outside of tox environments. #1574

adiroiban commented Mar 28, 2021 •

edited

Loading

adiroiban commented Mar 28, 2021 •

edited

Loading

adiroiban Mar 28, 2021

altendky Mar 30, 2021

altendky left a comment

altendky Mar 28, 2021

adiroiban Mar 29, 2021

altendky Mar 30, 2021

altendky Mar 28, 2021

adiroiban Mar 30, 2021

altendky Mar 28, 2021

adiroiban Mar 29, 2021

altendky Mar 30, 2021

altendky Mar 28, 2021

adiroiban commented Mar 29, 2021

adiroiban commented Mar 29, 2021

altendky left a comment

adiroiban commented Mar 30, 2021

adiroiban Mar 30, 2021

adiroiban Mar 30, 2021

adiroiban commented Mar 30, 2021

altendky left a comment

adiroiban commented Mar 31, 2021

		@@ -1 +0,0 @@
		repo_token: "JFDTIRUVOQ8jCM3zcajrZALlpKXyiXGAX"

		# A minimum dept of 2 is required, as for PR head is the auto-merge commit
		# and we need the last commit from PR to report the coverage.

	displayName: 'Generate XMl and report codecov.io'
	displayName: 'Generate XML and publish to Codecov'

[10159] Fix codecov.io reporting. Move codecov.io / coveralls reporting outside of tox environments. #1574

[10159] Fix codecov.io reporting. Move codecov.io / coveralls reporting outside of tox environments. #1574

Conversation

adiroiban commented Mar 28, 2021 • edited Loading

Scope and purpose

Changes

Contributor Checklist:

adiroiban commented Mar 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

altendky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiroiban commented Mar 29, 2021

adiroiban commented Mar 29, 2021

altendky left a comment

Choose a reason for hiding this comment

adiroiban commented Mar 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiroiban commented Mar 30, 2021

Coveralls fixes

Codecov

General changes

altendky left a comment

Choose a reason for hiding this comment

adiroiban commented Mar 31, 2021

adiroiban commented Mar 28, 2021 •

edited

Loading

adiroiban commented Mar 28, 2021 •

edited

Loading