Coveragepy: [5.3.1] Paths in file behind COVERAGE_PROCESS_START need to be resolved relative to that file's location (rather than current working directory)?

[hartwork]

Hi!

I wouldn't know what to do without coveragepy so thanks for releasing and sharing this software as Open Source with the community. :pray:

I have been able to track this issue down finally and I believe it's a bug in coveragepy. Let's see what you think.

Describe the Bug

Setup and Symptom (overview)

• I have two Python files, one calling the other using subprocess
• I have properly setup sub-processes tracking a la https://coverage.readthedocs.io/en/coverage-5.1/subprocess.html
• I have relative paths in .coveragerc as is convenient and common
• The working directory of the two processes differ (and that's key)
• The report shows 100% for the caller but the callee file keeps showing 0% coverage.

I'll reproduce that setup from the ground up as a Bash session for you to reproduce further down.

Cause

The paths in the coverage RF file pointed to by environment variable COVERAGE_PROCESS_START are currently resolved relative to the current working directory. So two processes with different working directiories do not agree on eventual file locations (as proven below). Putting an absolute path into COVERAGE_PROCESS_START a la COVERAGE_PROCESS_START="${PWD}"/.coveragerc does not change anything about the sitation.

Workaround

Once I make all paths absolute in .coveragerc things work as expected.
However that's not feasible most of the time and not more than a workaround.

To Reproduce

What follows is a single Bash shell session.
This is with Python 3.7 on Linux (but Python 3.9 is the same).

Let's create two simple Python programs, one saying hello, one calling the other.

$ cd "$(mktemp -d)"
$ echo $'#! /usr/bin/env python\nimport os, sys\nprint(f"Hello! (ARGV {sys.argv!r}, PWD {os.getcwd()!r})")' | tee callee.py
#! /usr/bin/env python
import os, sys
print(f"Hello! (ARGV {sys.argv!r}, PWD {os.getcwd()!r})")

$ echo $'#! /usr/bin/env python\nimport subprocess\nsubprocess.call(["sh", "-c", "cd /tmp && callee.py"])' | tee caller.py
#! /usr/bin/env python
import subprocess
subprocess.call(["sh", "-c", "cd /tmp && callee.py"])

$ chmod a+x caller.py callee.py
$ PATH="${PATH}:${PWD}" ./caller.py
Hello! (ARGV ['/tmp/tmp.hZ5VSTGIQT/callee.py'], PWD '/tmp')

My choice if /tmp is arbitrary here, all we need is some other directory.

Now let's add minimal coverage tracking with support for sub-processes as documented:

$ python3 -m venv venv
$ source venv/bin/activate
$ pip install coverage
$ echo $'try:\n\timport coverage\n\tcoverage.process_startup()\nexcept ImportError:\n\tpass' | tee "$(ls -1d venv/lib/python*)"/site-packages/sitecustomize.py
try:
        import coverage
        coverage.process_startup()
except ImportError:
        pass

$ echo $'[run]\nconcurrency = multiprocessing\ndata_file = ./.coverage\nomit = ./venv/*\nsource = ./' | tee .coveragerc
[run]
concurrency = multiprocessing
data_file = ./.coverage
omit = ./venv/*
source = ./

$ coverage erase ; COVERAGE_PROCESS_START="${PWD}"/.coveragerc PATH="${PATH}:${PWD}" coverage run ./caller.py ; coverage combine ; coverage report
Hello! (ARGV ['/tmp/tmp.hZ5VSTGIQT/callee.py'], PWD '/tmp')
Name        Stmts   Miss  Cover
-------------------------------
callee.py       2      2     0%  <-- !!!
caller.py       2      0   100%
-------------------------------
TOTAL           4      2    50%

What's to note here is that callee.py has a coverage of 0% reported while I want to see 100% here.

Let's add some selective debugging to see what's happening:

$ coverage erase ; COVERAGE_DEBUG=config,pid,process,trace COVERAGE_PROCESS_START="${PWD}"/.coveragerc PATH="${PATH}:${PWD}" coverage run ./caller.py |& grep -E 'source:|run_omit:|Matcher|cwd is|config_files_read|data_file|callee\.py' ; ls -l {,/tmp/}.coverage.* ; coverage combine ; coverage report ; rm -fv {,/tmp/}.coverage.*
18734.d7ff: cwd is now '/tmp/tmp.hZ5VSTGIQT'
18734.d7ff: Source matching against trees <TreeMatcher ['/tmp/tmp.hZ5VSTGIQT']>
18734.d7ff: Omit matching: <FnmatchMatcher ['/tmp/tmp.hZ5VSTGIQT/venv/*']>
18734.d7ff:               config_files_read: /tmp/tmp.hZ5VSTGIQT/.coveragerc
18734.d7ff:                       data_file: ./.coverage
18734.d7ff:                        run_omit: ./venv/*
18734.d7ff:                          source: ./
18734.d7ff: Source matching against trees <TreeMatcher ['/tmp/tmp.hZ5VSTGIQT']>
18734.d7ff: Omit matching: <FnmatchMatcher ['/tmp/tmp.hZ5VSTGIQT/venv/*']>
18734.d7ff:               config_files_read: /tmp/tmp.hZ5VSTGIQT/.coveragerc
18734.d7ff:                       data_file: ./.coverage
18734.d7ff:                        run_omit: ./venv/*
18734.d7ff:                          source: ./
18736.411c: cwd is now '/tmp'
18736.411c: New process: cmd: ['/tmp/tmp.hZ5VSTGIQT/callee.py']
18736.411c: Source matching against trees <TreeMatcher ['/tmp']>                         <-- !!!
18736.411c: Omit matching: <FnmatchMatcher ['/tmp/venv/*']>                              <-- !!!
18736.411c:               config_files_read: /tmp/tmp.hZ5VSTGIQT/.coveragerc
18736.411c:                       data_file: ./.coverage                                 <-- !!!
18736.411c:                        run_omit: ./venv/*
18736.411c:                          source: ./
18736.411c: Tracing '/tmp/tmp.hZ5VSTGIQT/callee.py'
Hello! (ARGV ['/tmp/tmp.hZ5VSTGIQT/callee.py'], PWD '/tmp')
-rw-r--r-- 1 user123 user123 53248 Jan  6 15:51 .coverage.hostname123.18734.675103
-rw-r--r-- 1 user123 user123 53248 Jan  6 15:51 /tmp/.coverage.hostname123.18736.622739  <-- !!!
Name        Stmts   Miss  Cover
-------------------------------
callee.py       2      2     0%  <-- !!!
caller.py       2      0   100%
-------------------------------
TOTAL           4      2    50%
removed '/tmp/.coverage.hostname123.18736.622739'

So what's happening is:

• The paths in .coveragerc are resolved relative to the current working directory.
• The two processes have different working directories
• One process ends up writing to /tmp/tmp.hZ5VSTGIQT/coverage.hostname123.18734.675103, the other to /tmp/.coverage.hostname123.18736.622739.

Let me demo a workaround by making all paths absolute in .coveragerc ourselves.
The sed-line below only works because I made sure that all paths start with a ./ prefix explicitly, earlier.

$ sed "s,\./,${PWD}/,g" -i .coveragerc  # make all relative paths absolute, -i requires GNU sed if you're on macOS
$ cat .coveragerc
[run]
concurrency = multiprocessing
data_file = /tmp/tmp.hZ5VSTGIQT/.coverage
omit = /tmp/tmp.hZ5VSTGIQT/venv/*
source = /tmp/tmp.hZ5VSTGIQT/

$ coverage erase ; COVERAGE_PROCESS_START="${PWD}"/.coveragerc PATH="${PATH}:${PWD}" coverage run ./caller.py ; coverage combine ; coverage report
Hello! (ARGV ['/tmp/tmp.hZ5VSTGIQT/callee.py'], PWD '/tmp')
Name        Stmts   Miss  Cover
-------------------------------
callee.py       2      0   100%  <-- !!!
caller.py       2      0   100%
-------------------------------
TOTAL           4      0   100%

Expected behavior

I hope that by now I have convinced you that relative paths in the covaragerc file pointed to by environment variable COVERAGE_PROCESS_START should be resolved relative to the location ofn the file, not the current working directory.

Thanks for your time and attention, best, Sebastian

[hartwork]

Any thoughts?

[nedbat]

Thanks for the detailed write-up! It sounds convincing. I will work through the reproduction soon.

[nedbat]

There are some subtle details to figure out if we are going to make this change.

• Should the new "files are relative to the location of .coveragerc" rule be applied all the time, or only when COVERAGE_PROCESS_START is in the mix?
• If all the time, then what about things like the COVERAGE_FILE environment variable, or the arguments to the Coverage() constructor?
• Are there cases that would break if we make this change? Should it be an option in the configuration?

[nedbat]

Just to clarify: this issue in particular is about the mention of the coverage data file, not the Python files that are being executed. There are four kinds of files mentioned in .coveragerc files. This change could apply to all of:

• Data file (.coverage)
• HTML report output directory
• XML report output file
• The lists of equivalent paths in the [paths] section.

[nedbat]

An idea from @fschulze (https://twitter.com/fschulze/status/1354423765225115648): coverage could define an environment variable with the directory configuration was read from. Then you could use that variable in your .coveragerc file.

Actually, come to think of it: you can define that environment variable yourself. Would that work for you? The simpler we can keep coverage.py, the better :)

[hartwork]

Hi Ned,

thank you for having a closer look at the issue!

There are some subtle details to figure out if we are going to make this change.

* Should the new "files are relative to the location of .coveragerc" rule be applied all the time, or only when COVERAGE_PROCESS_START is in the mix?

Probably all the time: If there is no guarantee what other path the relative paths in the file will be resolved against, then how will the user know what to put there?

* If all the time, then what about things like the COVERAGE_FILE environment variable, or the arguments to the Coverage() constructor?

Please elaborate, I'm not sure I have the full picture.

* Are there cases that would break if we make this change? Should it be an option in the configuration?

The case that breaks is when someone relies on paths to be relative to the current working directory.
Personally I'd favor a semver major version bump than an option because it's a bug and the new option will increase complexity further and multiprocess coverage is quite hard to get working properly, already. If using coverage is rocket science, only rocket scientists will get it to work :) If you want to be more conservative about it, then a new option with good documentation may be the least worst. The question would be when to turn the default from the old to new behavior then and you may need to duplicate parts of the test suite :thinking:

Just to clarify: this issue in particular is about the mention of the coverage data file, not the Python files that are being executed.

Are you sure? In my example above you can see that "Source matching" and "Omit matching" did not agree across process which to me seems like part of the bug. So I think it's a bout all relative paths in the file. Am I missing something?

An idea from @fschulze (https://twitter.com/fschulze/status/1354423765225115648): coverage could define an environment variable with the directory configuration was read from. Then you could use that variable in your .coveragerc file.

I'm mostly interested in clean solutions that work out of the box and match expectable behavior. Would this make the case I presented above work out of the box? Also, we don't seem to have support for environment variables inside .coveragerc yet (more below).

Actually, come to think of it: you can define that environment variable yourself. Would that work for you? The simpler we can keep coverage.py, the better :)

covagepy seems to use configparser.RawConfigParser which has interpolation of variables turned off, and even with some interpolation activated, none of the interpolations built-in with Python seem to support sourcing from environment variables; so we'd need a custom interpolation class here I guess.
On a side note, defining new variables into .coveragerc get's me error Unrecognized option '[run] prefix=' in config file .coveragerc but putting an absolute path into the file would be a non-solution anyway.

I'm looking forward to your reply.

Best, Sebastian

[nedbat]

Actually, come to think of it: you can define that environment variable yourself. Would that work for you? The simpler we can keep coverage.py, the better :)

covagepy seems to use configparser.RawConfigParser which has interpolation of variables turned off, and even with some interpolation activated, none of the interpolations built-in with Python seem to support sourcing from environment variables; so we'd need a custom interpolation class here I guess.

I appreciate the effort you are putting into this, but sometimes the docs are better than the code for understanding what is possible: coverage.py already does its own environment variable substitution in the configuration file: https://coverage.readthedocs.io/en/coverage-5.4/config.html#syntax

The more I think about this, the more I like it as a solution. I wonder what we can add to the docs to guide people to this solution.

[hartwork]

coverage.py already does its own environment variable substitution in the configuration file: https://coverage.readthedocs.io/en/coverage-5.4/config.html#syntax

Interesting! I confirm that (1) passing environment variable INITIAL_PWD="${PWD}" when calling coverage and (2) this config file work for a workaround:

[run]
concurrency = multiprocessing
data_file = ${INITIAL_PWD-.}/.coverage
omit = ${INITIAL_PWD-.}/venv/*
source = ${INITIAL_PWD-.}/

Please note that plain ${PWD-.} does not work because subprocesses can be shells that change ${PWD} to different values — I have tried.

The more I think about this, the more I like it as a solution. I wonder what we can add to the docs to guide people to this solution.

Personally, I consider it a workaround, not more; it is not by any means a solution to the problem of "unstable" path resolution.

[nedbat]

Personally, I consider it a workaround, not more; it is not by any means a solution to the problem of "unstable" path resolution.

I guess this gets us into debates about what "workaround" vs "solution" means, and whether something is "unstable". The path resolution is not unstable, it's just based on the current working directory rather than what you wanted. This feels to me like if we change it to be the configuration file's directory, some people will have broken builds because it worked for them to use the working directory.

In my mind, providing powerful primitives that can be used to create something that works for you is the best approach. It keeps coverage.py itself smaller, and provides the most flexibility.

[nedbat]

@arcivanov this issue is about the location of the data file, not a Python source file.

[arcivanov]

I'm sorry, I misread. Withdrawn.

[hartwork]

Personally, I consider it a workaround, not more; it is not by any means a solution to the problem of "unstable" path resolution.

I guess this gets us into debates about what "workaround" vs "solution" means, and whether something is "unstable". The path resolution is not unstable, it's just based on the current working directory rather than what you wanted. This feels to me like if we change it to be the configuration file's directory, some people will have broken builds because it worked for them to use the working directory.

Yes, it would be a breaking change. To my understanding, the underlying core question is "feature" vs. "bug". If a config file contains relative filenames and they are resolved to a path other than the very folder containing the config file, that's a bug to me. It's a feature to you, if I am not mistaken. If we disagree on that part, by design we will disagree about solution vs. workaround — right?

In my mind, providing powerful primitives that can be used to create something that works for you is the best approach. It keeps coverage.py itself smaller, and provides the most flexibility.

Flexible: yes. Little change to be done inside of coveragepy: yes. Consistent with reasonable user expectation: (arguably?) no. Works out of the box: no. Easy to explain to users: no. Easy to justify: ?

Maybe writing the docs explaining the current behavior, and why the user needs to put ${INITIAL_PWD-.} in their config file and pass INITIAL_PWD="${PWD}" in the environment, and why plain ${PWD} won't work, and why it's like that by design and when and all that — maybe writing that text for the docs will show if that's the right path or not. What do you think?