Skip to content

Automatically combine coverage in report#2162

Merged
nedbat merged 7 commits into
coveragepy:mainfrom
thatch:thatch/autocombine
May 5, 2026
Merged

Automatically combine coverage in report#2162
nedbat merged 7 commits into
coveragepy:mainfrom
thatch:thatch/autocombine

Conversation

@thatch
Copy link
Copy Markdown
Contributor

@thatch thatch commented Apr 25, 2026

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

@thatch thatch marked this pull request as ready for review April 25, 2026 02:23
Comment thread coverage/cmdline.py Outdated
if options.no_combine:
self.coverage.load()
else:
self.coverage.load()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Kind of disappointed Claude didn't factor this out!

@thatch
Copy link
Copy Markdown
Contributor Author

thatch commented Apr 25, 2026

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 report

but this is the least invasive way (it only changes the behavior of that first report, and does not introduce any new errors)

@nedbat
Copy link
Copy Markdown
Member

nedbat commented Apr 25, 2026

Sorry, which route? I'm missing something: with this code it's an error to combine if it's already been combined?

@thatch
Copy link
Copy Markdown
Contributor Author

thatch commented Apr 25, 2026

The choices for fixing this were:

  • just show a message
  • combine for the user (and save to disk)
  • combine for the user in memory only for reports
  • fail to report if coverage needs to be combined

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.

@nedbat
Copy link
Copy Markdown
Member

nedbat commented Apr 25, 2026

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.

@nedbat
Copy link
Copy Markdown
Member

nedbat commented Apr 27, 2026

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.

thatch added 7 commits April 28, 2026 03:36
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
@thatch thatch force-pushed the thatch/autocombine branch from 09c6db5 to 6cf5c84 Compare April 28, 2026 03:40
@thatch
Copy link
Copy Markdown
Contributor Author

thatch commented May 4, 2026

Done, PTAL

@nedbat nedbat merged commit 2d99fd7 into coveragepy:main May 5, 2026
44 checks passed
@nedbat
Copy link
Copy Markdown
Member

nedbat commented May 6, 2026

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.

@nedbat
Copy link
Copy Markdown
Member

nedbat commented May 7, 2026

I'm making the change to remove the data files during the implicit combine.

nedbat added a commit that referenced this pull request May 7, 2026
@nedbat
Copy link
Copy Markdown
Member

nedbat commented May 7, 2026

The removal is commit 1cd47aa, and the changelog entry (etc) is in c2a3a28.

@nedbat
Copy link
Copy Markdown
Member

nedbat commented May 10, 2026

This is now released as part of coverage 7.14.0.

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.

Add a hint that coverage combine might be needed

2 participants