Skip to content
This repository was archived by the owner on Feb 4, 2020. It is now read-only.

Let all tests calls use the same coverage file#215

Merged
frerich merged 3 commits into
frerich:masterfrom
webmaster128:appveyor-stuff
Aug 26, 2016
Merged

Let all tests calls use the same coverage file#215
frerich merged 3 commits into
frerich:masterfrom
webmaster128:appveyor-stuff

Conversation

@webmaster128
Copy link
Copy Markdown
Contributor

No description provided.

@webmaster128 webmaster128 force-pushed the appveyor-stuff branch 2 times, most recently from 4651e5b to 36c2780 Compare August 24, 2016 00:21
@codecov-io
Copy link
Copy Markdown

codecov-io commented Aug 24, 2016

Current coverage is 87.88% (diff: 100%)

Merging #215 into master will increase coverage by 10.78%

@@             master       #215   diff @@
==========================================
  Files             1          1          
  Lines           974        974          
  Methods           0          0          
  Messages          0          0          
  Branches        162        162          
==========================================
+ Hits            751        856   +105   
+ Misses          179         86    -93   
+ Partials         44         32    -12   

Powered by Codecov. Last update ec6ccec...3220340

@webmaster128 webmaster128 force-pushed the appveyor-stuff branch 2 times, most recently from eea80ae to 2300d55 Compare August 24, 2016 00:31
@webmaster128 webmaster128 changed the title Let all tests calls use the same coverage file [WIP] Let all tests calls use the same coverage file Aug 24, 2016
@webmaster128 webmaster128 force-pushed the appveyor-stuff branch 3 times, most recently from 57d9a1b to b798798 Compare August 24, 2016 06:20
Comment thread appveyor.yml Outdated

$clcachePy = "$((Get-Location).Path)\clcache.py"
$env:CLCACHE_CMD = "coverage run -a --branch --include=$clcachePy $clcachePy";
$env:CLCACHE_CMD = "coverage run --parallel-mode --branch --include=$clcachePy $clcachePy";
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part I don't understand - without -a, won't the integration test run overwrite the coverage data accumulated during the unit test run? And why is --parallel-mode needed?

@frerich
Copy link
Copy Markdown
Owner

frerich commented Aug 24, 2016

Aaah, good catch! So the tests wrote to different files? Judging by the nice increase in code coverage (and I see that e.g. reinvokePerSourceFile is now covered, too), I guess this fixes the issue with the coverage data which you noticed some time ago (and which I promised to tackle, ahem...).

@webmaster128
Copy link
Copy Markdown
Contributor Author

webmaster128 commented Aug 24, 2016

Yeah this was coming to my mind accedentaly yesterday. It somehow works but I do not understand exactly why. I'll post an update when I got it.

@webmaster128
Copy link
Copy Markdown
Contributor Author

We need --parallel-mode when running multiple coverage processes in parallel.

If you are measuring coverage in a multi-process program, or across a number of machines, you’ll want the --parallel-mode switch to keep the data separate during measurement. See Combining data files below.

http://coverage.readthedocs.io/en/coverage-4.0.3/cmd.html#execution

This happens in integrationtests.TestRunParallel when CLCACHE_CMD is the coverage command. Otherwise there will be race conditions when writing to that file.

With --parallel-mode set and COVERAGE_FILE unset, we get this file list

.coverage
coverage.xml
fibonacci.obj
integrationtests.xml
minimal.i
recompile1.obj
recompile2_custom_object_name.obj
tests/integrationtests/.coverage.APPVYR-WIN.1148.065970
tests/integrationtests/.coverage.APPVYR-WIN.1216.172713
tests/integrationtests/.coverage.APPVYR-WIN.1268.087382
tests/integrationtests/.coverage.APPVYR-WIN.1972.547268
tests/integrationtests/.coverage.APPVYR-WIN.2072.818891
tests/integrationtests/.coverage.APPVYR-WIN.2100.005178
tests/integrationtests/.coverage.APPVYR-WIN.2364.697755
tests/integrationtests/.coverage.APPVYR-WIN.2780.035160
tests/integrationtests/.coverage.APPVYR-WIN.684.302208
tests/integrationtests/.coverage.APPVYR-WIN.768.833310
tests/integrationtests/basedir/builddir_b/
tests/integrationtests/compiler-encoding/.coverage.APPVYR-WIN.1744.435794
tests/integrationtests/compiler-encoding/.coverage.APPVYR-WIN.672.182024
tests/integrationtests/fibonacci.exe
tests/integrationtests/fibonacci.obj
tests/integrationtests/fibonacci_c.exe
tests/integrationtests/fibonacci_c.obj
tests/integrationtests/fibonacci_cpp.exe
tests/integrationtests/fibonacci_cpp.obj
tests/integrationtests/header-change/.coverage.APPVYR-WIN.1964.359582
tests/integrationtests/header-change/.coverage.APPVYR-WIN.2084.208499
tests/integrationtests/header-change/.coverage.APPVYR-WIN.2244.296896
tests/integrationtests/header-change/.coverage.APPVYR-WIN.2752.430738
tests/integrationtests/header-change/main.exe
tests/integrationtests/header-change/main.obj
tests/integrationtests/header-miss-obsolete/.coverage.APPVYR-WIN.1960.661964
tests/integrationtests/header-miss-obsolete/.coverage.APPVYR-WIN.2092.141885
tests/integrationtests/header-miss-obsolete/.coverage.APPVYR-WIN.2592.269895
tests/integrationtests/header-miss/.coverage.APPVYR-WIN.2132.768007
tests/integrationtests/header-miss/.coverage.APPVYR-WIN.2148.753635
tests/integrationtests/header-miss/main.obj
tests/integrationtests/hits-and-misses/.coverage.APPVYR-WIN.1148.048555
tests/integrationtests/hits-and-misses/.coverage.APPVYR-WIN.2028.449413
tests/integrationtests/hits-and-misses/.coverage.APPVYR-WIN.2132.168966
tests/integrationtests/hits-and-misses/.coverage.APPVYR-WIN.2968.019754
tests/integrationtests/hits-and-misses/hit.obj
tests/integrationtests/minimal.obj
tests/integrationtests/minimal.pch
tests/integrationtests/mutiple-sources/.coverage.APPVYR-WIN.2472.217477
tests/integrationtests/mutiple-sources/.coverage.APPVYR-WIN.2704.529940
tests/integrationtests/mutiple-sources/.coverage.APPVYR-WIN.320.917832
tests/integrationtests/mutiple-sources/.coverage.APPVYR-WIN.684.284784
tests/integrationtests/parallel/.coverage.APPVYR-WIN.1172.016262
tests/integrationtests/parallel/.coverage.APPVYR-WIN.1232.976822
tests/integrationtests/parallel/.coverage.APPVYR-WIN.1288.763836
tests/integrationtests/parallel/.coverage.APPVYR-WIN.1460.767833
tests/integrationtests/parallel/.coverage.APPVYR-WIN.1556.587383
tests/integrationtests/parallel/.coverage.APPVYR-WIN.1676.950193
tests/integrationtests/parallel/.coverage.APPVYR-WIN.1680.568207
tests/integrationtests/parallel/.coverage.APPVYR-WIN.1764.456793
tests/integrationtests/parallel/.coverage.APPVYR-WIN.1768.684744
tests/integrationtests/parallel/.coverage.APPVYR-WIN.1864.121597
tests/integrationtests/parallel/.coverage.APPVYR-WIN.1964.047625
tests/integrationtests/parallel/.coverage.APPVYR-WIN.1972.297298
tests/integrationtests/parallel/.coverage.APPVYR-WIN.2012.317252
tests/integrationtests/parallel/.coverage.APPVYR-WIN.2072.946427
tests/integrationtests/parallel/.coverage.APPVYR-WIN.2124.658004
tests/integrationtests/parallel/.coverage.APPVYR-WIN.2132.728768
tests/integrationtests/parallel/.coverage.APPVYR-WIN.2136.787546
tests/integrationtests/parallel/.coverage.APPVYR-WIN.2216.543667
tests/integrationtests/parallel/.coverage.APPVYR-WIN.2260.448033
tests/integrationtests/parallel/.coverage.APPVYR-WIN.2268.335901
tests/integrationtests/parallel/.coverage.APPVYR-WIN.2316.233469
tests/integrationtests/parallel/.coverage.APPVYR-WIN.2404.682971
tests/integrationtests/parallel/.coverage.APPVYR-WIN.2424.762161
tests/integrationtests/parallel/.coverage.APPVYR-WIN.2580.051873
tests/integrationtests/parallel/.coverage.APPVYR-WIN.2580.474912
tests/integrationtests/parallel/.coverage.APPVYR-WIN.2632.743965
tests/integrationtests/parallel/.coverage.APPVYR-WIN.264.014561
tests/integrationtests/parallel/.coverage.APPVYR-WIN.2780.143438
tests/integrationtests/parallel/.coverage.APPVYR-WIN.2884.722990
tests/integrationtests/parallel/.coverage.APPVYR-WIN.2888.961276
tests/integrationtests/parallel/.coverage.APPVYR-WIN.3040.436297
tests/integrationtests/precompiled-headers/.coverage.APPVYR-WIN.2720.275318
tests/integrationtests/precompiled-headers/.coverage.APPVYR-WIN.2740.641090
tests/integrationtests/precompiled-headers/.coverage.APPVYR-WIN.3000.917830
tests/integrationtests/precompiled-headers/.coverage.APPVYR-WIN.576.143944
tests/integrationtests/precompiled-headers/applib.obj
tests/integrationtests/precompiled-headers/myapp.exe
tests/integrationtests/precompiled-headers/myapp.obj
tests/integrationtests/precompiled-headers/stable.pch
tests/integrationtests/vc120.pdb
tests/output/recompile2_custom_object_name.obj
tests/unittests/configuration/testDefaults.json
tests/unittests/configuration/testOpenClose.json
tests/unittests/manifests/06/
tests/unittests/manifests/8a/
tests/unittests/statistics/testHitCounts.json
tests/unittests/statistics/testOpenClose.json
unittests.xml

Now I want all .coverage.X.Y.Z files in one folder, but setting COVERAGE_FILE is the absolute file, not a prefix in parallel mode.

@webmaster128
Copy link
Copy Markdown
Contributor Author

ps.: -a (append) cannot be combined with --parallel-mode (tried it). It is either or.

@webmaster128 webmaster128 force-pushed the appveyor-stuff branch 3 times, most recently from 423cc52 to 4b4a6b9 Compare August 24, 2016 11:06
@webmaster128
Copy link
Copy Markdown
Contributor Author

Now this looks like a but on coverage.py to me – I'll report it.

@frerich
Copy link
Copy Markdown
Owner

frerich commented Aug 24, 2016

Or maybe it's an issue with how codecov.io aggregates the uploaded data? For what it's worth, the codecov.io author has been very responsive for me in the past, might well be worth checking back with him. I believe he's in the US time zone (he tends to respond at evening/night time by european standards).

@webmaster128
Copy link
Copy Markdown
Contributor Author

The issue is that COVERAGE_FILE somehow affects parallel mode: the directory changes. But COVERAGE_FILE is not a prefix for parallel mode files. So this should be a lower-level thing

@frerich
Copy link
Copy Markdown
Owner

frerich commented Aug 24, 2016

One thought - it might be neater to use the 'wip' label to denote that some PR is work in progress instead of mangling that information into the title of the PR. Just a cosmetic thing. :-)

@webmaster128
Copy link
Copy Markdown
Contributor Author

it might be neater to use the 'wip' label to denote that some PR is work in progress instead of mangling that information into the title of the PR. Just a cosmetic thing. :-)

Sure. But labels can only be set with additional permissions to the repository.

@frerich frerich added the wip label Aug 24, 2016
@frerich frerich changed the title [WIP] Let all tests calls use the same coverage file Let all tests calls use the same coverage file Aug 24, 2016
@webmaster128
Copy link
Copy Markdown
Contributor Author

Note to myself: Running integrationtests with set CLCACHE_CMD=coverage run --parallel-mode --branch --include=C:\\workspace\\clcache\\clcache.py C:\\workspace\\clcache\\clcache.py gives the following results.

  1. set COVERAGE_FILE= (unset): 90 files spead in subdirectories with names like .coverage.KulloWindows7VM.740.478041
  2. set COVERAGE_FILE=C:\\workspace\\clcache\\tests\\.cl_coverage: 91 files in the in the tests dir called like .cl_coverage.KulloWindows7VM.984.766105

In the first case, one file is missing because in TestBasedir.testBasedir an entire directory including a coverage file is removed.

This is 27 files less than the 118 files listed in AppVeyor.

@webmaster128
Copy link
Copy Markdown
Contributor Author

While python integrationtests.py respects the COVERAGE_FILE environment variable, py.test --junitxml .\integrationtests.xml integrationtests.py --cov=clcache does not.

This is because pytest-cov does some magic here, which seems to change COVERAGE_FILE for the child clcache processes.

@webmaster128 webmaster128 force-pushed the appveyor-stuff branch 2 times, most recently from c0a64fc to ee99e69 Compare August 26, 2016 08:08
@webmaster128 webmaster128 force-pushed the appveyor-stuff branch 3 times, most recently from e39e062 to f3bba1a Compare August 26, 2016 11:20
@webmaster128
Copy link
Copy Markdown
Contributor Author

I think I did it.

Comment thread .coveragerc_appveyor Outdated
# "Note that this plugin controls some options and setting the option in the
# config file will have no effect. These include specifying source to be
# measured (source option) and all data file handling (data_file and parallel
# options)."
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this file (almost) looks like the existing .coveragerc file. Couldn't we modify that to use the full path to clcache.py and thus omit all the --rcfile and --cov.config arguments?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if we can avoid the Appveyor-specific absolute file path ...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like include can be relative and pytest-cov deals with the rest. I'll strip this down accordingly.

@webmaster128
Copy link
Copy Markdown
Contributor Author

okay, cleaned up everything

Comment thread integrationtests.py Outdated
CLCACHE_CMD = os.environ['CLCACHE_CMD'].split()
else:
CLCACHE_CMD = [PYTHON_BINARY, CLCACHE_SCRIPT]
# pytest-cov note: subprocesses are covereage tested by default with some limitations
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor typo here: "covereage" -> "coverage"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@frerich frerich removed the wip label Aug 26, 2016
Simon Warta added 3 commits August 26, 2016 22:42
The cache statistics shown in this step do not represent what happened
in the tests anymore, since a lot of tests already use customs cache
folders.
@frerich frerich merged commit d0780d8 into frerich:master Aug 26, 2016
@frerich
Copy link
Copy Markdown
Owner

frerich commented Aug 26, 2016

Great job - both the unthankful job of figuring out how these tools work (I remember that it ways quite a pain to set it up...) and for the actual patches. Merged!

@webmaster128 webmaster128 deleted the appveyor-stuff branch August 26, 2016 20:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants