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

Report correct coverage.py data for tests that invoke subprocesses #56187

Open
ncoghlan opened this issue May 2, 2011 · 10 comments
Open

Report correct coverage.py data for tests that invoke subprocesses #56187

ncoghlan opened this issue May 2, 2011 · 10 comments
Labels
tests Tests in the Lib/test dir type-feature A feature request or enhancement

Comments

@ncoghlan
Copy link
Contributor

ncoghlan commented May 2, 2011

BPO 11978
Nosy @ncoghlan, @nedbat, @ultimatecoder
PRs
  • [WIP] bpo-11978: Run coverage with subprocess coverage and in parallel #1865
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = None
    created_at = <Date 2011-05-02.13:46:09.479>
    labels = ['type-feature']
    title = 'Report correct coverage.py data for tests that invoke subprocesses'
    updated_at = <Date 2020-01-24.23:32:56.556>
    user = 'https://github.com/ncoghlan'

    bugs.python.org fields:

    activity = <Date 2020-01-24.23:32:56.556>
    actor = 'brett.cannon'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = []
    creation = <Date 2011-05-02.13:46:09.479>
    creator = 'ncoghlan'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 11978
    keywords = []
    message_count = 9.0
    messages = ['134967', '134968', '291218', '291219', '291221', '291242', '294672', '296735', '298399']
    nosy_count = 3.0
    nosy_names = ['ncoghlan', 'nedbat', 'jaysinh.shukla']
    pr_nums = ['1865']
    priority = 'normal'
    resolution = None
    stage = None
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue11978'
    versions = []

    @ncoghlan
    Copy link
    Contributor Author

    ncoghlan commented May 2, 2011

    http://nedbatchelder.com/code/coverage/subprocess.html describes how to instruct a test suite that spawns subprocesses to correctly report coverage data for code covered only in those subprocesses.

    regrtest currently doesn't do this, so modules that use subprocesses to run their tests may report inaccurate coverage numbers when using this tool (as recommended in the devguide). (Those numbers are irredeemably inaccurate when it comes to regrtest's own coverage assessment)

    It may be better to build explicit invocation of coverage.py when regrtest is run under coverage.py into test.script_helper rather than trying to use the implicit mechanism though (as neither sitecustomize nor a .pth file are particularly good ideas when running Python's own test suite).

    @ncoghlan ncoghlan added the type-feature A feature request or enhancement label May 2, 2011
    @nedbat
    Copy link
    Member

    nedbat commented May 2, 2011

    Let me know if I can help.

    @ultimatecoder
    Copy link
    Mannequin

    ultimatecoder mannequin commented Apr 6, 2017

    I found the regrtest wasn't displaying correct coverage for when the code is executed from call Lib.test.support.script_helper.assert_python_ok. I found assert_python_ok is using subprocess under the hood. It seems this problem is unobserved from a long time but it is having status open.

    What should be the ideal situation for this? I request any core-developer to guide on this. I am interested in doing some more research on this issue. Thanks!

    @ncoghlan
    Copy link
    Contributor Author

    ncoghlan commented Apr 6, 2017

    As a starting point, I'd suggest looking at what can be achieved without making any changes to CPython or its test suite:

    1. Set COVERAGE_PROCESS_START in the environment where the tests are being run

    2. Inject a sitecustomize.py file into Lib (and add [Lib/sitecustomize.py](https://github.com/python/cpython/blob/main/Lib/sitecustomize.py) to .gitignore)

    There are cases that won't cover (like subprocesses with a custom environment), but it will provide a starting point for the tests that just pass the current environment through, and will also provide a way to notify test.support.script_helper of the expected value of COVERAGE_PROCESS_START in the future.

    @ncoghlan
    Copy link
    Contributor Author

    ncoghlan commented Apr 6, 2017

    To be more specific regarding sitecustomize.py:

        $ echo "print('Hello from sitecustomize.py')" > Lib/sitecustomize.py
        $ ./python -c "print('Hello from command line')"
        Hello from sitecustomize.py
        Hello from command line

    Since we only need these instructions to work for a local checkout, we can rely on the sitecustomize.py hook.

    It means we'll still miss coverage results from subprocess tests run in isolated mode or with site.py processing disabled, but those are both relatively rare and involve *not* running code that is normally run, so shouldn't impact the aggregate coverage results.

    @brettcannon
    Copy link
    Member

    There's also what changes might occur in the coverage results when we start using fullcoverage: python/core-workflow#18

    @brettcannon
    Copy link
    Member

    I learned a few thing trying to make this work.

    One is that the COVERAGE_PROCESS_START does need to point to an actual .ini file (which includes an empty file).

    Two, https://bitbucket.org/ned/coveragepy/src/63dfe482a849e9cb0b82214242315a806a6a3d53/coverage/control.py?at=default&fileviewer=file-view-default#control.py-1265 shows that stdlib coverage is left off by this, so even if you do turn on tracing in a subprocess, it will still ignore the stdlib. That means the coverage config file must turn on stdlib coverage in order or it to be picked up in the subprocesses.

    Three, you need to run coverage combine to create a unified .coverage file for coverage report to pick up (I don't know if codecov explicitly needs the merge step.

    When I have time I'll make a PR to test if this change actually helps speed things up.

    @brettcannon brettcannon self-assigned this May 29, 2017
    @brettcannon
    Copy link
    Member

    Just an FYI that I have never managed to make this work.

    @brettcannon brettcannon removed their assignment Jun 23, 2017
    @nedbat
    Copy link
    Member

    nedbat commented Jul 15, 2017

    Whoever takes this up next: let me know if I can help.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @iritkatriel iritkatriel added the tests Tests in the Lib/test dir label Nov 28, 2023
    @ambv
    Copy link
    Contributor

    ambv commented Feb 21, 2024

    So there was progress here. Ever since we switched to sys.monitoring, the default -j switch that uses multiple processes for parallelizing tests works correctly with gathering coverage. Now, we don't do that in the case where a test itself internally spawns another subprocess. I guess fixing this is in scope for this issue so I'll keep it open.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    tests Tests in the Lib/test dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants