test: fix test pollution in home directory #25
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
skillport/tests/unit/test_doc.py
Lines 6 to 7 in ad59f05
Update imports after sync->doc rename
The sync command module was renamed to doc.py, but callers still import skillport.interfaces.cli.commands.sync (e.g., app.py/init.py and this test). Because that module path no longer exists, importing the CLI or running the tests now throws a ModuleNotFoundError before any commands run, breaking the CLI entry point and the renamed test suite until the imports are pointed at the new module.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| output = tmp_path / "AGENTS.md" | ||
| result = runner.invoke(app, ["sync", "-o", str(output), "--force"]) | ||
| result = runner.invoke(app, ["doc", "-o", str(output), "--force"]) | ||
|
|
There was a problem hiding this comment.
The integration tests were updated to invoke skillport doc after the rename, but the Typer app still registers only a sync command (app.py line 185). Running these tests or invoking skillport doc will fail immediately with Typer’s “No such command ‘doc’” exit code, so the new tests cannot pass and users cannot access the intended command until the app registration is renamed or aliased.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR fixes test pollution by preventing tests from writing to ~/.skillport/ during test execution. The fix patches SKILLPORT_HOME to use tmp_path in the affected test, ensuring all test data remains in temporary directories.
Key Changes
- Added
SKILLPORT_HOMEpatching intest_add_derives_db_and_meta_from_skills_dirto redirect index/meta files to tmp_path - Fixed docstring with non-English characters to use English
- Renamed test class from
TestSyncCommandtoTestDocCommandand updated all test method names accordingly
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/integration/test_cli_commands.py | Added SKILLPORT_HOME patching to prevent test pollution; renamed sync tests to doc tests; fixed docstring with non-English characters |
| tests/unit/test_doc.py | New unit test file for doc command functionality |
| src/skillport/interfaces/cli/commands/doc.py | New command implementation file (appears to be renamed/refactored from sync command) |
Comments suppressed due to low confidence (8)
tests/unit/test_doc.py:12
- The import path references
skillport.interfaces.cli.commands.syncbut based on the file structure, this should be importing fromskillport.interfaces.cli.commands.docsince the new file isdoc.py, notsync.py.
src/skillport/interfaces/cli/commands/doc.py:165 - The function is named
syncbut the file is nameddoc.pyand the integration tests have been renamed fromTestSyncCommandtoTestDocCommandwith test methods renamed fromtest_sync_*totest_doc_*. This function should be renamed todocto be consistent with the command name.
src/skillport/interfaces/cli/commands/doc.py:173 - [nitpick] The parameter is named
sync_allbut since the command is being renamed fromsynctodoc, this should be renamed todoc_allorall_filesfor consistency.
src/skillport/interfaces/cli/commands/doc.py:285 - The success message says "Synced" which refers to the old
synccommand. This should be updated to match thedoccommand, e.g., "Generated documentation for {len(skills)} skill(s) to {out_path}" or "Updated {out_path} with {len(skills)} skill(s)".
src/skillport/interfaces/cli/commands/doc.py:3 - The module docstring mentions "sync コマンド" which contains non-English characters. Since this file is named "doc.py" and appears to implement the doc command (based on the test renames), this should be updated to "doc command" or similar in English.
tests/unit/test_doc.py:1 - The module docstring says "Unit tests for sync command" but based on the file name and the integration test renames (sync → doc), this should be "Unit tests for doc command".
src/skillport/interfaces/cli/commands/doc.py:212 - The docstring says "Sync skills to AGENTS.md" but based on the test renames and file name (
doc.py), this should be updated to reflect thedoccommand, e.g., "Generate skills documentation for AGENTS.md".
src/skillport/interfaces/cli/commands/doc.py:1 - The module docstring says "Sync installed skills to AGENTS.md" which refers to the old
synccommand name. This should be updated to reflect thedoccommand, e.g., "Generate documentation for installed skills in AGENTS.md".
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
| # Override SKILLPORT_HOME to use tmp_path instead of ~/.skillport | ||
| import skillport.shared.config as config_mod | ||
|
|
There was a problem hiding this comment.
Module 'skillport.shared.config' is imported with both 'import' and 'import from'.
Summary
Test plan