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

WIP: Codecov integration #170

Merged
merged 5 commits into from
Jul 5, 2019
Merged

WIP: Codecov integration #170

merged 5 commits into from
Jul 5, 2019

Conversation

takluyver
Copy link
Member

Try to collect code coverage information & report it through codecov. This is what the results look like: https://codecov.io/gh/pypa/auditwheel/branch/codecov

I think this is only reporting coverage from the unit tests at the moment. I'd like to try to figure out including coverage from the integration tests which run auditwheel inside docker containers, but that will probably be a bit more involved.

I did have to switch to an editable install in Travis. It seems awkward to match up the executed code with the source files otherwise.

@codecov
Copy link

codecov bot commented Jun 18, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@6fdab9f). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #170   +/-   ##
=========================================
  Coverage          ?   87.64%           
=========================================
  Files             ?       19           
  Lines             ?      963           
  Branches          ?      210           
=========================================
  Hits              ?      844           
  Misses            ?       83           
  Partials          ?       36

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fdab9f...b12da19. Read the comment docs.

@mayeut
Copy link
Member

mayeut commented Jun 18, 2019

Thanks for initiating that !

I think this is only reporting coverage from the unit tests at the moment. I'd like to try to figure out including coverage from the integration tests which run auditwheel inside docker containers, but that will probably be a bit more involved.

Yep, that might be tricky and that's one point I did not mention in #163, it would be good to know where there are holes in the tests before trying to fill them more or less blindly.

@lkollar
Copy link
Contributor

lkollar commented Jun 19, 2019

This is great. I was going to add Codecov myself at some point as the coverage is being built up, as so far the only indicator has been the output of pytest.

@lkollar
Copy link
Contributor

lkollar commented Jun 19, 2019

I just realised that Travis is not using tox at all. Another potential enhancement for the future would be to run the tests in Travis through tox so 1. there would be no duplication and 2. the same steps could be executed locally. I plan to look into this later.

@mayeut
Copy link
Member

mayeut commented Jun 26, 2019

I have a ugly hack that works for "docker coverage" i.e. I just wanted to know if/how it worked. I'll share the diff here but I can push a branch if needed (or to this branch for further improvement/review).

diff --git a/.coveragerc b/.coveragerc
new file mode 100644
index 0000000..13312b6
--- /dev/null
+++ b/.coveragerc
@@ -0,0 +1,4 @@
+[paths]
+source =
+    auditwheel/
+    /auditwheel_src/auditwheel
diff --git a/tests/integration/test_manylinux.py b/tests/integration/test_manylinux.py
index 70283af..21e4851 100644
--- a/tests/integration/test_manylinux.py
+++ b/tests/integration/test_manylinux.py
@@ -62,7 +62,7 @@ def docker_start(image, volumes={}, env_variables={}):
     dvolumes = {host: {'bind': ctr, 'mode': 'rw'}
                 for (ctr, host) in volumes.items()}
 
-    logger.info("Starting container with image %r", image)
+    logger.info("Starting container with image %r, env %r", image, env_variables)
     con = client.containers.run(image, ['sleep', '10000'], detach=True,
                                 volumes=dvolumes, environment=env_variables)
     logger.info("Started container %s", con.id[:12])
@@ -75,6 +75,9 @@ def docker_container_ctx(image, io_dir, env_variables={}):
     if src_folder is None:
         pytest.skip('Can only be run from the source folder')
     vols = {'/io': io_dir, '/auditwheel_src': src_folder}
+    for key in env_variables:
+        if src_folder in env_variables[key]:
+            env_variables[key] = env_variables[key].replace(src_folder, '/auditwheel_src')
 
     container = docker_start(image, vols, env_variables)
     try:
@@ -111,10 +114,15 @@ def any_manylinux_container(request, io_folder):
     policy = request.param
     image = MANYLINUX_IMAGES[policy]
     env = {'PATH': PATH[policy]}
+    for key in os.environ:
+        if key.startswith('COV_CORE_'):
+            logger.info('%s=%s', key, os.environ[key])
+            env[key] = os.environ[key]
+    env['COV_CORE_DATAFILE'] = env['COV_CORE_DATAFILE'] + '.docker'
 
     with docker_container_ctx(image, io_folder, env) as container:
-        docker_exec(container, 'pip install -U pip setuptools')
-        docker_exec(container, 'pip install -U /auditwheel_src')
+        docker_exec(container, 'pip install -U pip setuptools pytest pytest-cov')
+        docker_exec(container, 'pip install -U -e /auditwheel_src')
         yield policy, container

Now: pytest -s --cov auditwheel --cov-branch

---------- coverage: platform darwin, python 3.7.2-final-0 -----------
Name                                       Stmts   Miss Branch BrPart  Cover
----------------------------------------------------------------------------
auditwheel/__init__.py                         0      0      0      0   100%
auditwheel/__main__.py                         4      4      2      0     0%
auditwheel/condatools.py                      22      0      2      0   100%
auditwheel/elfutils.py                        81      4     48      5    93%
auditwheel/genericpkgctx.py                   10      5      6      1    38%
auditwheel/hashfile.py                         8      0      2      0   100%
auditwheel/lddtree.py                        140      7     78     11    92%
auditwheel/main.py                            34      4      6      2    85%
auditwheel/main_addtag.py                     34     24      6      0    25%
auditwheel/main_lddtree.py                    11      3      0      0    73%
auditwheel/main_repair.py                     43      7     16      5    80%
auditwheel/main_show.py                       48      7     25      4    79%
auditwheel/policy/__init__.py                 47      1     20      1    97%
auditwheel/policy/external_references.py      42      0     22      1    98%
auditwheel/policy/versioned_symbols.py        26      1     19      1    96%
auditwheel/repair.py                          87     10     37      7    85%
auditwheel/tmpdirs.py                         37      9      6      1    67%
auditwheel/tools.py                           57     11     20      2    78%
auditwheel/wheel_abi.py                      119      1     66      1    99%
auditwheel/wheeltools.py                     124      7     46      6    92%
----------------------------------------------------------------------------
TOTAL                                        974    105    427     48    87%

============= 49 passed, 1 warnings in 615.99 seconds ===========

@takluyver
Copy link
Member Author

Nice, I thought it would be more effort than that. The only thing that I'm not quite sure about is replacing src_folder in all the environments - can we do it only for COV_CORE_* variables?

Is it sufficient to install coverage in the container rather than pytest-cov? Or is pytest-cov doing something of its own with subprocesses?

@mayeut
Copy link
Member

mayeut commented Jun 28, 2019

The only thing that I'm not quite sure about is replacing src_folder in all the environments - can we do it only for COV_CORE_* variables?

Sure. As I said, it was a ugly hack just to check it works, I went to the place where I found the string needed to be replaced and just replaced everything.
After more digging around it seems that the line + env['COV_CORE_DATAFILE'] = env['COV_CORE_DATAFILE'] + '.docker' is not needed

Is it sufficient to install coverage in the container rather than pytest-cov? Or is pytest-cov doing something of its own with subprocesses?

pytest-cov is indeed doing something of its own with subprocesses. It's rather transparent when all runs on the same machine but not so much with docker. There are python hooks installed by pytest-cov that do some things at python start in presence of COV_CORE_* variables. It's probably best to rely on pytest-cov than trying to reimplement something with coverage (and have it play nicely with the host which runs pytest-cov)

References:

@takluyver
Copy link
Member Author

Great. No problem with using pytest-cov in the containers - I was just interested what it was doing.

@takluyver
Copy link
Member Author

If you want to turn your work into commits, feel free to add them to this branch.

@mayeut
Copy link
Member

mayeut commented Jun 28, 2019

Will do

@mayeut
Copy link
Member

mayeut commented Jun 29, 2019

@takluyver, I did something bad I think, I rebased the branch and forced pushed it. It's in my basic workflow of doing things. When working alone on a branch, it's alright to do so but since you're obviously working on it too, I messed things up in the git history for you...
A git reset --hard HEAD~2 then git pull should do I guess.

@mayeut
Copy link
Member

mayeut commented Jun 29, 2019

Since I already forced push once and then, there were conflict for merging, I rebased and forced push once more.

@mayeut mayeut closed this Jun 29, 2019
@mayeut mayeut deleted the codecov branch June 29, 2019 15:32
@mayeut mayeut restored the codecov branch June 29, 2019 15:33
@mayeut mayeut reopened this Jun 29, 2019
@mayeut
Copy link
Member

mayeut commented Jun 30, 2019

I merged the master branch (instead of rebasing) for codecov to process correctly coverage (seems it has trouble with rebasing). We can squash all commits once we're ok with this PR.

@auvipy auvipy merged commit ed684aa into master Jul 5, 2019
@takluyver takluyver deleted the codecov branch July 5, 2019 18:32
@takluyver
Copy link
Member Author

Thanks for following up on this - I didn't have a chance to look at it until now, but happy to see it merged.

@takluyver
Copy link
Member Author

And we're already doing pretty well on coverage, it seems. :-) The biggest chunk of code going unexercised is the function for the auditwheel addtag subcommand.

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

Successfully merging this pull request may close these issues.

4 participants