| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
| if options.no_combine: | ||
| self.coverage.load() | ||
| else: | ||
| self.coverage.load() |
There was a problem hiding this comment.
Kind of disappointed Claude didn't factor this out!
Sorry, something went wrong.
|
I went this route because if coverage is already combined, coverage combine is an error? In case people are doing coverage report
coverage combine
coverage reportbut this is the least invasive way (it only changes the behavior of that first report, and does not introduce any new errors) |
Sorry, something went wrong.
|
Sorry, which route? I'm missing something: with this code it's an error to combine if it's already been combined? |
Sorry, something went wrong.
|
The choices for fixing this were:
I chose the third option (the "route I went"), because it's the most helpful one that won't break someone's workflow if they already correctly use coverage combine. For example: ick$ coverage run -p -m pytest ick$ ls .coverage* .coverage.rygel.pid516142.X36NpWex.HJ0Mof874Lyh .coverage.rygel.pid516286.XJ8maiIx.HMo3vtrY00vh .coverage.rygel.pid516287.X2ZksaEx.Hpqw0TtWiBLh .coverage.rygel.pid516311.XM9ZYrrx.HNQIp837rWOh .coverage.rygel.pid516314.XzudWwcx.HNQIp837rWOh ick$ coverage combine Combined data file .coverage.rygel.pid516142.X36NpWex.HJ0Mof874Lyh Combined data file .coverage.rygel.pid516286.XJ8maiIx.HMo3vtrY00vh Combined data file .coverage.rygel.pid516287.X2ZksaEx.Hpqw0TtWiBLh Combined data file .coverage.rygel.pid516311.XM9ZYrrx.HNQIp837rWOh Skipping duplicate data .coverage.rygel.pid516314.XzudWwcx.HNQIp837rWOh ick$ coverage combine No data to combine ick$ echo $? 1 If I made coverage report do a combine (just like the existing combine does, saving back to disk), it would make a later coverage combine now error out because it's already been done. I thought that was too backwards-incompoatible to do the second choice; the first choice wouldn't have saved me much frustration. The fourth choice also doesn't feel great. |
Sorry, something went wrong.
|
I see, thanks. Quickly reading the code, I had missed that the combined data was not written out. My only concern with combining but not saving is that coverage report; coverage json; coverage html will combine three times, but explicitly combining first will fix that, and we can explain it in the docs. |
Sorry, something went wrong.
|
I think we can do without --no-combine. I can't think of a reason why someone would want to skip combining, and if they really need to, they can copy data files around, or ask us later to add the option. |
Sorry, something went wrong.
This does not modify the state on disk, but one common newbie error is doing `coverage run ...` followed by `coverage report` which shows some, but not the true coverage. Default to doing the combine, but allow people to pass `--no-combine` if this breaks their workflows. Fixes coveragepy#1781
|
Done, PTAL |
Sorry, something went wrong.
|
We have a problem. If the test cycle is run; report, the parallel data files are kept. The next run; report will combine twice as many files. The third cycle will have three times as many. Nothing is cleaning up the data files. With the old run; combine; report, the combine step turned .coverage.* into .coverage, overwriting the previous .coverage. I think it will be error-prone for these files to hang around, so we should re-consider having the implicit combine keep the data files. What would be bad about the implicit combine deleting the parallel files? Doing that would also mean that run; report; html wouldn't combine twice. |
Sorry, something went wrong.
|
I'm making the change to remove the data files during the implicit combine. |
Sorry, something went wrong.
Sorry, something went wrong.
|
This is now released as part of coverage 7.14.0. |
Sorry, something went wrong.
|
Hi, this is a very sensible feature, but guess what: it breaks our workflow! (Please let me know if you'd rather I opened this as a separate ticket.) We currently run tests over several modules and report them separately. Then we report the combined coverage over all modules, i.e. cd modules/one
coverage run --parallel-mode
coverage combine --keep
coverage report --fail-under=80
# Files:
# modules/one/.coverage
# modules/one/.coverage.abc
# modules/one/.coverage.def
cd ../two
coverage run --parallel-mode
coverage combine --keep
coverage report --fail-under=80
# Files:
# modules/two/.coverage
# modules/two/.coverage.ghi
# modules/two/.coverage.jkland then: coverage combine modules/*
coverage reportwill combine the modules/one/.coverage.* and modules/two/.coverage.* files into a single .coverage file. I can see the logic on combining for a report, as it saves a step in the majority of cases, but it's also less flexible. I've had to rollback to 7.13.5 for now, as copying the files first is more file management than I'd like (given that it worked before). I can think of a few solutions:
Option 2 would allow us to skip our first coverage combine-steps completely. |
Sorry, something went wrong.
|
I've added --keep-combined to the reporting commands in commit f24a91f. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This does not modify the state on disk, but one common newbie error is
doing coverage run ... followed by coverage report which shows some,
but not the true coverage.
Default to doing the combine, but allow people to pass --no-combine if
this breaks their workflows.
Fixes #1781