Skip to content

test: fix help docs test by adding _import_module to fake kernel#59

Open
zza-830 wants to merge 5 commits into
TruFoundation:mainfrom
zza-830:fix/help-docs-test
Open

test: fix help docs test by adding _import_module to fake kernel#59
zza-830 wants to merge 5 commits into
TruFoundation:mainfrom
zza-830:fix/help-docs-test

Conversation

@zza-830

@zza-830 zza-830 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Problem

test_run_help_prints_docstring_for_known_command was failing across all open PRs because the fake kernel in the test lacked a _import_module method. When run_help tried to import the settings module, it hit an AttributeError that was silently caught, falling through to "No detailed help available".

Fix

Added a working _fake_import_module function to the test's SimpleNamespace fake kernel that uses importlib.import_module() to resolve the module path. This allows run_help to successfully load the module, read the docstring, and print it.

Verification

tests/test_help_docs.py::test_run_help_prints_docstring_for_known_command PASSED

Fixes the CI failure affecting PRs #50, #51, #53, #54.

Summary by CodeRabbit

  • Tests

    • Updated the help-docs test to use the real settings module and its docstring for assertions, with a small adjustment to internal test stubbing/formatting.
  • Documentation

    • Removed an in-code explanatory comment in the command-normalization area (no change to the underlying behavior).

Ziang Zhang added 2 commits June 8, 2026 14:31
The test_run_help_prints_docstring_for_known_command test was failing
because the fake kernel lacked a _import_module method, causing the
module import to fail silently and fall through to the error message.

Add a working _fake_import_module that uses importlib to resolve
the module path, allowing run_help to read and print the docstring.
The  function referenced a bare name  that was
never imported or defined, causing a NameError crash on every invocation.

Fix: move  before the conditional block so it is
always defined when reached by the comparison at the end of the function.

Closes TruFoundation#40
@AkshajSinghal

Copy link
Copy Markdown
Collaborator

Great find!
But I'd request you to implement these changes:

  1. Add error handling for malformed quoting in _os_passthrough.
  2. Add a Windows path test case to ensure spaces/backslashes are handled correctly.
  3. Update the README to accurately describe the new sanitization process.

@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c028d558-8d4c-4b5a-9c62-7bff4185616b

📥 Commits

Reviewing files that changed from the base of the PR and between 2bbe1f2 and 823b3b4.

📒 Files selected for processing (1)
  • tests/test_help_docs.py

📝 Walkthrough

Walkthrough

Test kernel setup refactored to use real module imports via a _fake_import_module helper instead of stubs, validating help output against the actual run_settings docstring. Multi-line explanatory comment removed from CLI normalization path; no logic changes.

Changes

Test kernel refactoring and CLI cleanup

Layer / File(s) Summary
Test kernel setup refactoring with real module imports
tests/test_help_docs.py
Added importlib import and reworked fake_kernel setup with a _fake_import_module helper that loads the real module from registry path mapping. Wired this helper into fake_kernel._import_module to validate help text against the actual run_settings docstring instead of a stubbed function.
CLI comment removal
trushell/cli.py
Removed a multi-line explanatory comment immediately before the lowercase command/argument normalization in app_with_lower; runtime behavior unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • TruFoundation/TruShell#58: Both PRs touch tests/test_help_docs.py to validate help output driven by the kernel's registry (one adds registry/module-import-based tests; the other refines the test to use the real run_settings docstring/module import plumbing).

Poem

🐰 A comment fades from the CLI's way,
Real modules now light the test's day,
No stubby fakes, just truth laid bare,
Help docs verified with utmost care. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding _import_module to the fake kernel to fix a failing test in test_help_docs.py.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch fix/help-docs-test

Comment @coderabbitai help to get the list of available commands and usage tips.

@zza-830

zza-830 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review! This PR is specifically a test fix — it adds _import_module to the fake kernel in test_help_docs.py so the help docstring test passes. The _os_passthrough error handling and Windows path test you mentioned are outside the scope of this PR, but great ideas for follow-up work.

Merge conflict with upstream/main has been resolved. Ready for another look!

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_help_docs.py`:
- Line 3: Remove the dead helper and unused import: delete the unused function
_fake_import_module and its assignment to fake_kernel._import_module, and remove
the importlib import since it's only referenced by that dead function; keep the
working mock assignment (the lambda that returns fake_module) as the sole
_import_module replacement so tests still pass.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 70442d9b-75ad-4969-84f6-6989b2ef371f

📥 Commits

Reviewing files that changed from the base of the PR and between 7ab2e53 and 5050b4d.

📒 Files selected for processing (2)
  • tests/test_help_docs.py
  • trushell/cli.py
💤 Files with no reviewable changes (1)
  • trushell/cli.py

Comment thread tests/test_help_docs.py
zza-830 pushed a commit to zza-830/TruShell that referenced this pull request Jun 9, 2026
- add_task now only splits on quotes when args actually starts with
  a quote character. Plain multi-word input like 'buy groceries' is
  treated as the full task text, not split into task + category.
- Remove dead _fake_import_module function from test_help_docs.py
  (was overridden by the lambda on the next line).

Addresses CodeRabbit review on TruFoundation#53 and TruFoundation#59.
Ziang Zhang added 2 commits June 9, 2026 18:50
The previous version used a simple lambda mock that bypassed the
import-path conversion contract. Restored _fake_import_module which
verifies that file paths like 'trushell/commands/settings.py' are
correctly converted to importable module paths.

Addresses CodeRabbit review on TruFoundation#59.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants