Opened 4 years ago

Closed 4 years ago

#8266 enhancement closed fixed (fixed)

Twisted's coverage reports should include the scripts that are run in subprocesses

Reported by: hawkowl Owned by: hawkowl
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/cov-subprocess-8266-3
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl

Description

With the Python 3 on Windows port, some of these will need to be changed. We should have these be covered by coverage, so we're sure we're not breaking things.

Change History (9)

comment:1 Changed 4 years ago by hawkowl

Author: hawkowl
Branch: branches/cov-subprocess-8266

(In [47262]) Branching to cov-subprocess-8266.

comment:2 Changed 4 years ago by hawkowl

Branch: branches/cov-subprocess-8266branches/cov-subprocess-8266-2

(In [47289]) Branching to cov-subprocess-8266-2.

comment:3 Changed 4 years ago by hawkowl

Keywords: review added

This is good, I think.

The tox env now passes the correct environment variables, and copies out the coverage files from the tox env once it's run. The user now has to run coverage combine/report, but we shouldn't document our coverage testing process until things like https://bitbucket.org/hpk42/tox/issues/275/allow-running-commands-before-and-after are fixed and it makes sense.

See the 12.04 builder https://buildbot.twistedmatrix.com/builders/ubuntu12.04-py2.7-coverage/builds/22, which runs the tox env. After this lands, all builders will be migrated to it.

Reasons for some of the changes:

  1. To make coverage collection + reporting work, we can't just execute the data file. We have to do it with -m so it recognises that it is part of the Twisted package. This requires passing through the environment properly.
  1. The -u makes Python use unbuffered I/O, which makes some tests less flaky.
  1. The .pth hack is from http://nedbatchelder.com/blog/201001/running_code_at_python_startup.html .

comment:4 Changed 4 years ago by Adi Roiban

Can we have a comment on admin/_copy.py describing its role

same for admin/zz_coverage.pth

Rather than having a comment in the trac ticket about the .pth hack I would prefer to have it as a comment inside the file.


I am not very happy with the way we use tox. it looks like we are using it both as a test runner and as a build tool.

but I am happy to see that we have removed some test related logic from buildbot and have it inside the code branch.


Can we have a simple ./admin/ci.py script which is called by buildbot using various arguments.

so instead of 3 clearing virtualenv + 4 installing tox + 5 clearing coverage, build will just call ./admin/ci.py before ...

similar instead of 7 run coverage combine + 8 run coverage html + 10 run coverage xml + 11 upload to codecov maybe we can have ./admin/ci.py after

the upload can remain as a buildbot step


What is properEnv and why do we want to use a non-properEnv ? :)


I would say that this can be merged and I am happy to see more test running logic contained in the branch.

Leaving this for review so that another dev can check this.

For me it looks good to merge... with the above minor comments.

Many thanks!

comment:5 Changed 4 years ago by Glyph

Keywords: review removed
Owner: set to hawkowl

Abandon your fear, adi!

I am removing this from review because it has feedback, and we should get out of the habit of passing the buck to other developers. I haven't looked at it, I assume that adi's feedback is sensible.

Hawkowl, please address the points raised above and resubmit. Thanks!

comment:6 Changed 4 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

Hi, thanks for the review.

  1. Comment added about the pth file in it, and in the _copy.py.
  1. I think we're using it reasonably; it's an automation tool, not just a dedicated test runner. Build tasks are part of its documented use.
  1. I don't know about a ci.py - the good thing about them being encoded in the buildbot (and later, the travis config) is that then the individual steps have logs and it's not just a massive dump to the stdio.
  1. Added a comment about properEnv to the relevant files.

As per Glyph, putting back up for review. Builders spun.

comment:7 Changed 4 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Thanks!

  1. I am not happy with tox, since to automate things you need hacks like admin/_copy.py and I think that the .ini format is abused here.

Since we still delegate to external python scripts and hacks, why not replace

    coverage: python {toxinidir}/admin/_copy.py {toxinidir}/admin/zz_coverage.pth {envsitepackagesdir}/zz_coverage.pth
    coverage: coverage erase
    coverage: coverage run -p --rcfile={toxinidir}/.coveragerc {envbindir}/trial --reactor={env:TWISTED_REACTOR:default} {posargs:twisted}
    coverage: python {toxinidir}/admin/_copy.py "_trial_temp/.coverage*" {toxinidir}
    coverage: python {toxinidir}/admin/_copy.py ".coverage*" {toxinidir}

with just

    coverage: ./admin/coverage --test={posargs:twisted}

in this way tox.ini is our CI entry point with a clean delegation


  1. I think that we can work around a convention for buildbot to break each step into separate logs

The ugly part about buildbot steps is that if you want to add or modify a step you need to go the twisted-admin route ... and manual testing if that step is much complicated

With a simple script, you can just set up the CI env on your local system and try to run the command in a similar way as buildbot.


  1. Maybe properEnv can be renamed to bytesOnlyEnv or something more descriptive.

Good to merge.

Thanks!

comment:8 Changed 4 years ago by hawkowl

Branch: branches/cov-subprocess-8266-2branches/cov-subprocess-8266-3

(In [47360]) Branching to cov-subprocess-8266-3.

comment:9 Changed 4 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [47362]) Merge cov-subprocess-8266-3: Twisted's coverage reports should include the scripts that are run in subprocesses

Author: hawkowl Reviewer: adiroiban Fixes: #8266

Note: See TracTickets for help on using tickets.