-
-
Notifications
You must be signed in to change notification settings - Fork 459
WIP, BUG: futures with dir switching #2069
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
base: master
Are you sure you want to change the base?
Conversation
* Fixes nedbatgh-2065. * Add a regression test for the failure to collect `.coverage` files when parent and child (`concurrent.futures`) processes have different directory contexts. This test fails for the expected reason it seems. * Add a patch that fixes the problem described in the matching issue, but for some reason does not manage to restore the desired behavior in the regression test (something I'm not understanding about the "meta" testing?)
| for fname in os.listdir(): | ||
| if fname.startswith(".coverage."): | ||
| if not os.path.exists(os.path.join(self.orig_dir, fname)): | ||
| shutil.copy(fname, self.orig_dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line here does the trick to restore coverage of the missing add() function in the example below the fold when I install this feature branch locally:
import os
import tempfile
from concurrent.futures import ProcessPoolExecutor
def add(a, b):
return a + b
def probe_dispatcher():
orig_dir = os.getcwd()
with tempfile.TemporaryDirectory() as temp_dir:
os.chdir(temp_dir)
dispatcher()
os.chdir(orig_dir)
def dispatcher():
futures = []
with ProcessPoolExecutor() as executor:
futures.append(executor.submit(add, 2, 2))
for future in futures:
future.result()
if __name__ == "__main__":
probe_dispatcher()
whereas suppressing that shutil.copy() line restores the original bug:
While I can reproduce this lack of coverage with the new regression test I added here, I can't make that test pass on this feature branch at the moment... am I missing something about the meta testing/coverage testing harness?
| d = original_get_preparation_data(name) | ||
| d["stowaway"] = Stowaway(rcfile) | ||
| with open(".coverpath", "w", encoding="utf-8") as cpath: | ||
| cpath.write(d["orig_dir"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels kind of bad... even more configuration/hacking via the multiprocessing monkeypatching, but it does seem to work as part of storing the original working directory for the originally described issue, so that .coverage. files may be copied back there.
| nprocs = 1 | ||
| upto = 30 | ||
| code = (MULTI_CODE_DIR_CHANGE).format(NPROCS=nprocs, UPTO=upto) | ||
| total = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
total is unused now since I'm not really wanting to write output from my sample test case... but perhaps more importantly I can't make this new regression test pass at the moment, despite the installed version of this feature branch doing the right thing for the reproducing external example as noted at https://github.com/nedbat/coveragepy/pull/2069/files#r2446215263. Am I missing something about the meta-testing/test harness here?
Fixes BUG: tmpdir directory context and ProcessPoolExecutor prevent tracing report #2065.
Add a regression test for the failure to collect
.coveragefiles when parent and child (concurrent.futures) processes have different directory contexts. This test fails for the expected reason it seems.Add a patch that fixes the problem described in the matching issue, but for some reason does not manage to restore the desired behavior in the regression test (something I'm not understanding about the "meta" testing?)