Replace deprecated isolated_filesystem() with tmp_path in tests - #1226
Conversation
d7d7d59 to
c1789fb
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The changes remove deprecated test helpers to satisfy -Werror runs, and the only noted issue is a minor reduction in coverage fidelity for one “default directory” test case.
Pull request overview
This PR updates the test suite to avoid Click’s deprecated CliRunner.isolated_filesystem() helper (which now triggers warnings) by switching tests to use pytest-managed temporary paths/directories, keeping the nox test session clean under -Werror.
Changes:
- Update unit/integration CLI tests to write inputs/outputs under pytest-provided temp directories (
tmp_path) rather than Click’sisolated_filesystem(). - Adjust CLI invocations to use explicit file/directory paths where needed.
- Replace
isolated_filesystem()usage in mosaics CLI integration tests with a temporary directory + explicitchdir.
File summaries
| File | Description |
|---|---|
| tests/unit/test_cli_collect.py | Use tmp_path for writing an input JSON file and invoke CLI with that path. |
| tests/integration/test_orders_cli.py | Replace filesystem isolation with tmp_path-based download directories across orders download tests. |
| tests/integration/test_mosaics_cli.py | Replace Click isolation with tempfile.TemporaryDirectory() and manual chdir for file-producing CLI tests. |
| tests/integration/test_data_cli.py | Use tmp_path for asset download directory setup and assertions. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| runner = CliRunner() | ||
| with runner.isolated_filesystem() as folder: | ||
| result = invoke(['download', oid], runner=runner) | ||
| assert result.exit_code == 0 | ||
| result = invoke(['download', '--directory', str(tmp_path), oid], | ||
| runner=runner) | ||
| assert result.exit_code == 0 |
There was a problem hiding this comment.
+1 -- --directory defaults to . so passing it explicitly means the default path (as in not specifying --directory) is no longer covered. Could you update this test so that it does not specify --directory? Perhaps monkeypatch.chdir() would be helpful here, i.e.:
@respx.mock
def test_cli_orders_download_default(invoke,
mock_download_response,
oid,
tmp_path,
monkeypatch):
mock_download_response()
monkeypatch.chdir(tmp_path)
runner = CliRunner()
result = invoke(['download', oid], runner=runner)
assert result.exit_code == 0There was a problem hiding this comment.
The same pattern applies to test_cli_orders_download_checksum and test_cli_orders_download_overwrite which also add --directory, but those tests are not testing the default download command so I think they are safe to leave as-is with the added --directory param.
| def run_test(tc: CLITestCase): | ||
| runner = CliRunner() | ||
| with runner.isolated_filesystem() as folder: | ||
| for r in tc.requests: | ||
| r() | ||
|
|
||
| args = ["mosaics", "-u", baseurl] + tc.command + tc.args | ||
| result = runner.invoke(cli.main, args=args) | ||
| with tempfile.TemporaryDirectory() as folder: | ||
| prev_dir = os.getcwd() | ||
| os.chdir(folder) | ||
| try: | ||
| for r in tc.requests: | ||
| r() | ||
|
|
||
| args = ["mosaics", "-u", baseurl] + tc.command + tc.args | ||
| result = runner.invoke(cli.main, args=args) | ||
| finally: | ||
| os.chdir(prev_dir) |
There was a problem hiding this comment.
nit: method run_test() is also called in https://github.com/planetlabs/planet-client-python/blob/main/tests/integration/test_mosaics_api.py#L40. It works in this MR because the signature is unchanged and the os.chdir call is scoped to run_test(). We could make this more durable though by creating a fixture that uses Pytest's monkeypatch.chdir, i.e. see this pseudo-code example I created with Claude:
# tests/integration/conftest.py
@pytest.fixture
def cwd_tmp_path(tmp_path, monkeypatch):
"""Run the test with the cwd set to a fresh tmp_path, restored afterwards."""
monkeypatch.chdir(tmp_path)
return tmp_path
# test_mosaics_cli.py
def test_cli(tc: CLITestCase, cwd_tmp_path):
run_test(tc, cwd_tmp_path)
@respx.mock
def run_test(tc: CLITestCase, folder):
runner = CliRunner()
for r in tc.requests:
r()
...
# test_mosaics_api.py
def test_api(tc, cwd_tmp_path):
api = async_wrap(MosaicsAPI)
with patch('planet.cli.mosaics.MosaicsClient', api):
test_mosaics_cli.run_test(tc, cwd_tmp_path)
api._pool.shutdown()There was a problem hiding this comment.
Great idea, I have pushed changes in line with this suggestion, thanks!
…empfile - test_cli_orders_download_default: use monkeypatch.chdir(tmp_path) and omit --directory to actually test the default download path behavior - Add cwd_tmp_path fixture to integration conftest.py using monkeypatch.chdir - run_test in test_mosaics_cli.py now takes folder as a parameter, removing manual os.chdir/restore and tempfile.TemporaryDirectory usage - test_mosaics_api.py passes cwd_tmp_path through to run_test
d32b2f3 to
ee8b266
Compare
isolated_filesystemis being deprecated and now manifests as warnings in the tests. The test stage fails since we run with-Werror. We can use thetmp_pathfixture from pytest instead.PR Checklist: