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

adiroiban · 2021-03-09T14:55:21Z

Scope and purpose

We got random failures on pypy (both 3.6 and 3.7 versions)

Also pypy jobs with coverage are very slow...30minutes.

See what can be done to speed up the CI.

Changes

PYPY coverage

Coverage was disabled for most of the PyPy..
The tests are much faster now without coverage 4m vs 30m
We still run coverage on PYPY, but only for compat and a few other test that are exercising the PYPY specific code.

In this way we can monitor if we continue to see random CI failures at the end of the test run on PY.

These are the tests required to get the extra coverage on pypy

twisted.test.test_compat
twisted.test.test_defer
twisted.internet.test.test_socket
twisted.trial.test.test_tests

Linux / MacOS general CI speedup

All tests are now executed in parallel, including pypy.

To gain more speed, I have removed tox --notests as it was just wasting time.
We can later show the dependencies using the python-info action.

PR are waiting to be reviewed to also get parallel jobs on Windows.

Coverage reporting

I have updated coverage XML generation and codecov.io reporting to make the check red on failures.
Only coveralls failures are ignored for now.
In this way it should be easier to observer coverage issues.

I tried to fix the coveralls report for trunk merges.
I did some work in #1574 but there is no way to test this unless merged in trunk.

Contributor Checklist:

The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/10121
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)
[NA] I have updated the automated tests.
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.

wsanchez · 2021-03-15T17:34:39Z

codecov/project shows a slight drop. I'm getting a 403 error when I try to view it on Codecov, but it would be good to know where the dropped lines are.

adiroiban · 2021-03-31T09:57:21Z

codecov.yml

@@ -12,4 +19,12 @@ coverage:
      default:
        target: 100%

-comment: off
+# We don't want to receive general PR comments about coverage.


These are already the defaults, but I have added them here to make it explicit and help undestand how the whole codecov.io process works

adiroiban · 2021-03-31T09:58:08Z

codecov.yml

+  require_ci_to_pass: yes
+  notify:
+    after_n_builds: 7
+    wait_for_ci: yes


Here I am trying to see if I can configure codecov.io to wait a bit more before sending the coverage reports so that we don't have transient red checks

I don't think the existing behavior is bad, necessarily, it's just confusing for people who (like me) assume that a check is complete once it shows either a check or X.

In my experience these two options don't actually work. I never got to the bottom of why and codecov.io declined to assist.

I will try to find some time and report this issue via the internal / paid support codecov channel.

It is true that codecov are not helping too much with open source projects... so we are on a free ride here.

But I see if I can reproduce it for my private project and try to get help via that project.

adiroiban · 2021-03-31T10:51:45Z

I have no idea why codecov/project is red ... maybe it will need time to be updated -
i have checked the page https://app.codecov.io/gh/twisted/twisted/compare/1543/changes and the PR has no missing coverage (when compared to trunk)

This is ready for review. See PR description

twm · 2021-04-01T08:22:08Z

.github/workflows/test.yaml

      run: |
        # sub-process coverage are generated in separate files so we combine them
        # to get an unified coverage for the local run.
        # The XML is generate to be used with 3rd party tools like diff-cover.
        python -m coverage combine
        python -m coverage xml -o coverage.xml -i
-        python -m coverage report
+        python -m coverage report --skip-covered


twm · 2021-04-01T08:26:17Z

.github/workflows/test.yaml

@@ -36,6 +36,8 @@ jobs:
        # As of 9 March 2021 GHA VM have 2 CPUs - Azure Standard_DS2_v2
        # Trial distributed jobs enabled to speed up the CI jobs.
        trial-args: ['-j 4']


Should this be removed as you removed the reference to it above?

adiroiban · 2021-04-01T09:22:24Z

@twm thanks for the review. I have updated the GHA config. Let me know if you see any issues with it.

I see the codecov.io check is still sent before all checks are done

The codecov.io docs say that cryptography project is a good example
https://docs.codecov.io/docs/merging-reports
https://github.com/pyca/cryptography/blob/main/codecov.yml

The codecov/project is now red ...but I guess that after some time codecov.io is preocessing the reports and will update it.

The codecov page looks green https://app.codecov.io/gh/twisted/twisted/compare/1543/changes

I happened with my previous comment. By the time I left the comment the check was red for a5326cc and now it's green

My plan is to rewrite the coverage reporting in a separate PR, so that we collect all GHA coverage in a separate job and combine ourselves and then send a single coverage.xml for the whole GHA builds... I will test in in a separate branch ... but might need moving one Windows and MacOS job to GitHub

wiml · 2021-04-01T23:28:48Z

The codecov/project is now red

It looks like the change is due to a now-untested branch in TestDecoratorMixin.test_decorateTestSuiteReferences(), near lines 1167–1169, which skips a test if sys.getrefcount is not available … I'd guess that PyPy is the only tested target that doesn't have getrefcount. Maybe add twisted.trial.test.test_tests (or all of twisted.trial) to the mypy suite?

adiroiban · 2021-04-02T13:26:39Z

It looks like the change is due to a now-untested branch in TestDecoratorMixin.test_decorateTestSuiteReferences(),

Thanks. You are my hero :)

Updated pypy coverage only take 3 minutes..

7 minutes is the total run time for GitHub Actions tests.

We still have Azure tests which take 14m.

So with the changes from this PR GHA should be ok and we should look at Azure speedup next

wiml

LGTM, although I'm not terribly familiar with codecov.io+GHA interactions.

…nto 10121-pypy-no-coverage

exarkun · 2021-04-05T14:01:22Z

codecov.yml

+        # Future test runs will not trigger the error and the error handling
+        # code branch is not called, and if no other tests is touching that
+        # branch we are left with overall reduced code coverage.
+        threshold: 0.02%


This setting may be required if coverage is a required check but the reasoning here is invalid. The threshold value allows an ongoing slip in coverage. Build N of trunk can have (0.02 × M) greater coverage than build (N+M) of trunk and this configuration tells codecov.io that this is fine.

So this setting should be considered a temporary fix while the real problem is being addressed - non-deterministic coverage in the test suite.

There are various cases where coverage can be reduced. Even just deleting lines that have been covered causes this. It's just a result of not having '100%' coverage (after including consideration of coverage ignores). I don't know if there's a way to deal with that separately from the commented situation.

What are the odds, that is the case in my next review. twisted/towncrier#334 They deleted a line that is useless and the coverage dropped by 0.01%.

Thanks wiml for the review. Just monitoring future PR and checking for overall project coverage regression can help with future fixed for this issue.

Thanks exarkun for the headup. I forgot to file the followup ticket.

Thanks altendky for the code delete info. I was hoping that codeco.io is smart and will not trigger such an error.
I have reported this issue
https://community.codecov.io/t/project-coverage-decrease-on-deleted-line/2726

I agree that this should be a temporary change.
I have added to link to the ticket in the comment.

If coverage/project will continue to be red, people will most probably ignore it by default.
With that, I think that is a higher probability of seeing an overall decrease in coverage.

We had the project red for some time, and I am not aware of any secondary tickets or actions to fix the overall project coverage.

Regarding 0.02 x M.
We have 100% patch coverage , so is not that easy to find a series of PR with 100% patch coverage but witch will always decreased the overall project by 0.02 due to different tests.

I am monitoring overall coverage changes as there is a gang of usual suspect tests.

For example

With this swinging line

There is also this swinging test helpers to prevent a flaky test failure

altendky

To gain more speed, I have removed tox --notests as it was just wasting time.

I guess that should be brought back in #1577 after this PR is merged?

altendky · 2021-04-05T14:15:03Z

codecov.yml

+        # Future test runs will not trigger the error and the error handling
+        # code branch is not called, and if no other tests is touching that
+        # branch we are left with overall reduced code coverage.
+        threshold: 0.02%


There are various cases where coverage can be reduced. Even just deleting lines that have been covered causes this. It's just a result of not having '100%' coverage (after including consideration of coverage ignores). I don't know if there's a way to deal with that separately from the commented situation.

adiroiban · 2021-04-05T20:29:34Z

I guess that should be brought back in #1577 after this PR is merged?

@altendky yep... looking forward for that PR. It would be nice to see a separate timing for deps install vs actual test run. Thanks

adiroiban force-pushed the 10121-pypy-no-coverage branch 3 times, most recently from 9ffbc37 to 03e42c1 Compare March 9, 2021 15:40

adiroiban force-pushed the 10121-pypy-no-coverage branch 2 times, most recently from bc18121 to e542130 Compare March 18, 2021 23:20

adiroiban changed the title ~~[10121] Disable coverage reporting for PyPy.~~ [10121] Fix random PyPy CI failures. Mar 24, 2021

adiroiban force-pushed the 10121-pypy-no-coverage branch 4 times, most recently from bbec8bc to 9300a7d Compare March 24, 2021 18:47

adiroiban mentioned this pull request Mar 30, 2021

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

Open

8 tasks

Disable coverage for pypy. Enable parallel run for pypy.

6243c19

adiroiban force-pushed the 10121-pypy-no-coverage branch from 37f9a03 to 6243c19 Compare March 31, 2021 00:13

Run subset of pypy coverage.

4c52eec

adiroiban force-pushed the 10121-pypy-no-coverage branch from 8745b5d to 4c52eec Compare March 31, 2021 01:02

Skip separate tox --notest to save more than 1 minute.

f5625eb

adiroiban changed the title ~~[10121] Fix random PyPy CI failures.~~ [10121] Fix random PyPy CI failures and improve GHA CI speed. Mar 31, 2021

Run fewer coverage tests on pypy.

da0afe6

adiroiban mentioned this pull request Mar 31, 2021

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

Merged

5 tasks

adiroiban commented Mar 31, 2021

View reviewed changes

adiroiban added 2 commits March 31, 2021 11:03

Wait longer for codecov.io.

85395de

Extend pypy coverage tests.

a5326cc

adiroiban changed the title ~~[10121] Fix random PyPy CI failures and improve GHA CI speed.~~ [10121] Fix random PyPy CI failures. Improve GHA CI speed. Improve coverage reporting. Mar 31, 2021

adiroiban requested a review from a team March 31, 2021 11:09

twm reviewed Apr 1, 2021

View reviewed changes

Clean gha workflow.

204b3b4

adiroiban requested a review from twm April 1, 2021 09:10

Update pypy trial coverage.

5612850

wiml approved these changes Apr 2, 2021

View reviewed changes

adiroiban and others added 5 commits April 4, 2021 00:20

Merge branch 'trunk' into 10121-pypy-no-coverage

eb4671f

Alow for a bit of slack in overall coverage.

b8cb2b1

Merge branch 'trunk' into 10121-pypy-no-coverage

ad45079

Merge branch '10121-pypy-no-coverage' of github.com:twisted/twisted i…

94850f3

…nto 10121-pypy-no-coverage

Add info about codecov.io checks.

c9b74d5

exarkun reviewed Apr 5, 2021

View reviewed changes

altendky approved these changes Apr 5, 2021

View reviewed changes

Add link to trac ticket.

a371b9c

adiroiban merged commit b3cf4b5 into trunk Apr 5, 2021

adiroiban deleted the 10121-pypy-no-coverage branch April 5, 2021 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

adiroiban commented Mar 9, 2021 •

edited

Loading

wsanchez commented Mar 15, 2021

adiroiban Mar 31, 2021

adiroiban Mar 31, 2021

wiml Apr 2, 2021

exarkun Apr 5, 2021

adiroiban Apr 5, 2021

adiroiban commented Mar 31, 2021

twm Apr 1, 2021

twm Apr 1, 2021

adiroiban commented Apr 1, 2021 •

edited

Loading

wiml commented Apr 1, 2021

adiroiban commented Apr 2, 2021

wiml left a comment

exarkun Apr 5, 2021

altendky Apr 5, 2021

altendky Apr 5, 2021

adiroiban Apr 5, 2021

altendky left a comment

altendky Apr 5, 2021

adiroiban commented Apr 5, 2021

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

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

Conversation

adiroiban commented Mar 9, 2021 • edited Loading

Scope and purpose

Changes

PYPY coverage

Linux / MacOS general CI speedup

Coverage reporting

Contributor Checklist:

wsanchez commented Mar 15, 2021

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 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiroiban commented Apr 1, 2021 • edited Loading

wiml commented Apr 1, 2021

adiroiban commented Apr 2, 2021

wiml 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

altendky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiroiban commented Apr 5, 2021

adiroiban commented Mar 9, 2021 •

edited

Loading

adiroiban commented Apr 1, 2021 •

edited

Loading