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

CoverageSqliteData.filename gets mutated with suffix #708

Closed
blueyed opened this issue Sep 16, 2018 · 8 comments
Closed

CoverageSqliteData.filename gets mutated with suffix #708

blueyed opened this issue Sep 16, 2018 · 8 comments

Comments

@blueyed
Copy link
Contributor

@blueyed blueyed commented Sep 16, 2018

While CoverageJsonData.filename will be kept as the initial basename, CoverageSqliteData.filename gets mutated with the suffix appended.

This is relevant for when using combine() on an already used Coverage object, where the suffix is included in the glob pattern then already when looking for files to combine.

This appears to be (one of) the reason(s) for pytest-cov to have a separate Coverage object for this (https://github.com/pytest-dev/pytest-cov/blob/7205428bfb9ab9f8dbcaeefe23ce5fe426bb06b9/src/pytest_cov/engine.py#L152-L155).

@nedbat

This comment has been minimized.

Copy link
Owner

@nedbat nedbat commented Sep 17, 2018

My understanding is that the changed behavior in pytest-cov==2.6.0 also happens with coverage.py 4.5.1, so I don't think the SQLite storage code is part of the cause.

@blueyed

This comment has been minimized.

Copy link
Contributor Author

@blueyed blueyed commented Sep 17, 2018

Yes, that's correct.
I've just noticed this when looking into it, and I think coveragepy's API for both storages should behave the same in this regard. That's why I've filed this issue only really.

IIRC I've looked into changing pytest-cov with regard to the new API (not really feasible, since coveragepy 4 should be supported of course).

As for pytest-django, where I've noticed and debugged this with I've gone with coveragepy directly now (pytest-dev/pytest-django@c72b10a).

@nedbat

This comment has been minimized.

Copy link
Owner

@nedbat nedbat commented Sep 22, 2018

To be honest, I would rather change .filename into ._filename. Help me understand why people are looking at .filename in the first place so I can get the API right.

@blueyed

This comment has been minimized.

Copy link
Contributor Author

@blueyed blueyed commented Sep 22, 2018

The use case is combining (see pytest-dev/pytest-cov#226 - pytest-cov uses a separate Coverage instance just for having an unmodified data_file IIRC).

When looking into this I've tried to create data_paths manually, but basename is not available, and therefore this did not work - that's when I've noticed the inconsistency (this issue) - but I think it is fine to reflect the actual filename (i.e. it could stay as-is).

Instead I think basename should be stored additionally on the data instances, and then get used by combine_parallel_data (instead of filename):

coveragepy/coverage/data.py

Lines 685 to 689 in 6c14ffb

def combine_parallel_data(data, aliases=None, data_paths=None, strict=False):
"""Combine a number of data files together.
Treat `data.filename` as a file prefix, and combine the data from all
of the data files starting with that prefix plus a dot.

The problem currently is that filename gets used when looking for file's to combine, and with CoverageSqliteData would contain the suffix then already:

coveragepy/coverage/data.py

Lines 711 to 712 in 6c14ffb

data_dir, local = os.path.split(data.filename)
localdot = local + '.*'

@nedbat

This comment has been minimized.

Copy link
Owner

@nedbat nedbat commented Sep 23, 2018

Heh, I ask for an example, and you show me my own code! :)

I'd like to write a failing test case for this. It seems like the scenario is: use a Coverage object with parallel=true to run some code, then try to use that same object to combine, and it won't find all the data files? I'm not sure that's a reasonable use case for the API, but help me understand.

@blueyed

This comment has been minimized.

Copy link
Contributor Author

@blueyed blueyed commented Sep 23, 2018

Yes, that't the use case.. :)

nedbat added a commit that referenced this issue Sep 24, 2018
nedbat added a commit that referenced this issue Sep 24, 2018
@nedbat

This comment has been minimized.

Copy link
Owner

@nedbat nedbat commented Sep 24, 2018

I added a failing test to show the problem. It passes with the JSON storage, and fails with SQLite. I'm not entirely convinced this is an important case to keep working, but it shouldn't be hard to fix either.

nedbat added a commit that referenced this issue Apr 15, 2019
@nedbat nedbat closed this in fdaf035 Apr 15, 2019
@nedbat

This comment has been minimized.

Copy link
Owner

@nedbat nedbat commented Apr 15, 2019

This is fixed in fdaf035

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.