Test suite fails on Windows: unescaped path regex in testArgPassing, NamedTemporaryFile reopen in testFileNameFire
- 主要言語
- Python
- スター
- 28.2k
- フォーク
- 1.5k
- PR マージ指標
- 30日以内にマージされた PR はありません
説明
On a fresh clone on Windows, the test suite fails 2 of 261 tests, both in `fire/main_test.py` and both Windows-specific. CI currently runs `macos-latest` + `ubuntu-latest` only, so neither can be caught there.
```
FAILED fire/main_test.py::MainModuleTest::testArgPassing - re.PatternError: bad escape \p at position 5
FAILED fire/main_test.py::MainModuleFileTest::testFileNameFire - PermissionError: [Errno 13] Permission denied: 'C:\\...\\tmponwjvj2u.py'
2 failed, 259 passed in 6.30s
```
Environment: Windows 11, Python 3.13.13, `pip install -e . pytest hypothesis`, clone at current master.
**1. `testArgPassing` — unescaped path in a regex.** `assertOutputMatches` treats its argument as a regex, and the test interpolates `os.path.join('part1', 'part2', 'part3')` unescaped. On Windows that's `part1\part2\part3`, and `\p` has been a hard error (`re.PatternError`) since Python 3.12. POSIX never sees it because `/` needs no escaping. The open PR #679 fixes exactly this with `re.escape()`; applying its change locally takes the suite to 1 failed / 260 passed, so it would be lovely to see it merged.
**2. `testFileNameFire` — `NamedTemporaryFile` can't be reopened while open on Windows.** `MainModuleFileTest.setUp` keeps the temp `.py` file open, and `fire.__main__.import_from_file_path` → `exec_module` then opens it a second time for import. That second open is the documented Windows limitation of `NamedTemporaryFile` ("the name cannot be used to open the file a second time while it is still open"), and it surfaces as `PermissionError` inside the import machinery. This is test-harness-only: `python -m fire somefile.py` itself works fine on Windows.
Fix I verified locally for (2) — with it (plus #679's change), the whole suite passes on this machine, `261 passed in 1.65s`, and it stays green on POSIX semantics:
```diff
def setUp(self):
super().setUp()
- self.file = tempfile.NamedTemporaryFile(suffix='.py') # pylint: disable=consider-using-with
+ self.file = tempfile.NamedTemporaryFile(suffix='.py', delete=False) # pylint: disable=consider-using-with
self.file.write(b'class Foo:\n def double(self, n):\n return 2 * n\n')
- self.file.flush()
+ self.file.close()
+ self.addCleanup(os.unlink, self.file.name)
- self.file2 = tempfile.NamedTemporaryFile() # pylint: disable=consider-using-with
+ self.file2 = tempfile.NamedTemporaryFile(delete=False) # pylint: disable=consider-using-with
+ self.file2.close()
+ self.addCleanup(os.unlink, self.file2.name)
```
(`delete_on_close=False` would be tidier but is 3.12+, and setup.py still advertises 3.7 support.)
I'm reporting this as an issue rather than sending the patch because I haven't signed the Google CLA yet. Happy for anyone — including the author of #679 — to fold the diff above into a PR. Also happy to follow up with a `windows-latest` line for the CI matrix discussion if that's of interest, though I understand that's a bigger decision than these two fixes.
コントリビューションガイド
調査の方向性
Start in fire/main_test.py at MainModuleTest.testArgPassing and MainModuleFileTest.setUp/testFileNameFire, then run the Windows test suite with pytest. Check the existing PR #679 context for the regex failure and verify the temporary files can be reopened during import. Done means all 261 tests pass on Windows and POSIX.
索引モデルが issue の本文から書いたものです。
評価
- 技術スタック
- python
- 領域
- testing-qa
- issue の種類
- バグ
- 難易度
- 2/5
- 見積もり時間
- 1〜3時間
- 活発さ
- 静か
- 明瞭さ
- 明確に書かれている
- 初心者へのやさしさ
- 85/100