download_test_pdfs silently swallows thread exceptions, masking flaky CI failures
nf-testing
`tests/__init__.py:128-141` calls `concurrent.futures.wait(futures)` but never iterates the returned futures or calls `.result()` on them. Any exception raised in a worker thread is silently captured and never surfaced.
### The code
```python
def download_test_pdfs() -> None:
pdfs = read_yaml_to_list_of_dicts(Path(__file__).parent / "example_files.yaml")
with concurrent.futures.ThreadPoolExecutor(max_workers=20) as executor:
futures = [
executor.submit(get_data_from_url, pdf["url"], name=pdf["local_filename"])
for pdf in pdfs
]
concurrent.futures.wait(futures)
```
### Symptom
When a download fails (flaky network, rate limit, external host hiccup), `download_test_pdfs()` returns without raising. The test suite then proceeds assuming every file is local. The first failure surfaces several seconds later as a pytest `FileNotFoundError` when a test tries to open a missing file, which masks the real cause (the actual download failure from minutes earlier).
### Observed impact
This surfaced in at least one Windows CI run: the cache restore missed, so `download_test_pdfs()` was triggered, and one of the fetches from `raw.githubusercontent.com` failed silently. The test that eventually opened `ASurveyofImageClassificationBasedTechniques.pdf` failed with a misleading `FileNotFoundError: ...tests/pdf_cache/ASurveyofImageClassificationBasedTechniques.pdf`. Investigating the root cause required correlating the pytest failure back to the network download step, which is more work than necessary for what is essentially "the host had a hiccup."
### Proposed fix
Iterate the futures and call `.result()` on each to re-raise worker thread exceptions:
```python
with concurrent.futures.ThreadPoolExecutor(max_workers=20) as executor:
futures = [
executor.submit(get_data_from_url, pdf["url"], name=pdf["local_filename"])
for pdf in pdfs
]
for future in concurrent.futures.as_completed(futures):
future.result() # re-raises any exception from the worker
```
If the intent is to tolerate partial failures, collect exceptions and raise a summary:
```python
errors = []
for future in concurrent.futures.as_completed(futures):
try:
future.result()
except Exception as e:
errors.append(e)
if errors:
raise RuntimeError(
f"Failed to download {len(errors)} test PDF(s). First error: {errors[0]}"
) from errors[0]
```
Either way, failures should be visible at the source, not masked as downstream FileNotFoundError.
### Context
Spotted while investigating a Windows CI flake on another PR. The symptom was a confusing `FileNotFoundError` on a test file that looked unrelated to the PR's changes. The actual root cause was this silent swallow plus a flaky external download, neither of which the PR could have caused.
2 条评论