test: add extension package-data contract coverage#108
Merged
Conversation
📝 WalkthroughWalkthroughTwo new contract test modules are added under ChangesExtensions Layer Contract Tests and Docs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/architecture/extensions-layer.md`:
- Around line 200-207: There is a contradiction in the extensions-layer.md
documentation regarding packaging requirements for non-`.py` extension assets.
The status note (`#99`) at lines 205-207 states that no additional
`force-include`/`include` entries were required in `pyproject.toml` for the test
case, but earlier bullets in the same section claim such entries are necessary
for non-`.py` extension assets. Review both statements to determine the actual
packaging contract and revise whichever statement is inaccurate so that the
documentation clearly and consistently explains when and why
`force-include`/`include` entries are or are not required in `pyproject.toml`
for extension assets.
In `@tests/extensions/test_extensions_package_data_contract.py`:
- Around line 102-106: The condition checking if result.returncode != 0 should
not automatically skip the test with pytest.skip() because this masks real build
failures and packaging regressions. Instead, distinguish between the build tool
being unavailable versus an actual build failure. Keep the skip only for cases
where the build tool itself is truly unavailable in the environment, but fail
the test hard (using assert or pytest.fail) when the build command actually runs
but returns a non-zero exit code, as this indicates a real packaging problem
that should not pass CI.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bd032be9-116d-4cc6-a110-d0ac37bd3e3d
📒 Files selected for processing (3)
docs/architecture/extensions-layer.mdtests/extensions/test_extensions_package_data_contract.pytests/extensions/test_extensions_tree_contract.py
Comment on lines
+200
to
+207
| - **Status (#99):** tree and package-data contract tests live in | ||
| `tests/extensions/` (`test_extensions_tree_contract.py`, | ||
| `test_extensions_package_data_contract.py`). They prove representative | ||
| imported extension assets are present in the repository tree and inside the | ||
| built wheel and sdist. Hatchling's default file selection for | ||
| `packages = ["src/ecli"]` already ships the imported data files, so **no | ||
| additional `force-include`/`include` entries were required** in | ||
| `pyproject.toml`. |
There was a problem hiding this comment.
Reconcile this status note with the earlier packaging rule in the same section.
Lines 205-207 say no additional force-include/include entries were required, but earlier bullets still state such entries are required for non-.py extension assets. Please align both statements so the packaging contract is unambiguous.
🤖 Prompt for 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.
In `@docs/architecture/extensions-layer.md` around lines 200 - 207, There is a
contradiction in the extensions-layer.md documentation regarding packaging
requirements for non-`.py` extension assets. The status note (`#99`) at lines
205-207 states that no additional `force-include`/`include` entries were
required in `pyproject.toml` for the test case, but earlier bullets in the same
section claim such entries are necessary for non-`.py` extension assets. Review
both statements to determine the actual packaging contract and revise whichever
statement is inaccurate so that the documentation clearly and consistently
explains when and why `force-include`/`include` entries are or are not required
in `pyproject.toml` for extension assets.
Comment on lines
+102
to
+106
| if result.returncode != 0: | ||
| pytest.skip( | ||
| "package build unavailable in this environment " | ||
| f"(exit {result.returncode}):\n{result.stdout}\n{result.stderr}" | ||
| ) |
There was a problem hiding this comment.
Do not skip when the build command fails.
On Line 102, converting a real build failure into pytest.skip(...) masks packaging regressions and can let broken artifact contracts pass CI. Keep the skip only for “frontend unavailable” and fail hard on non-zero exit.
Proposed fix
- if result.returncode != 0:
- pytest.skip(
- "package build unavailable in this environment "
- f"(exit {result.returncode}):\n{result.stdout}\n{result.stderr}"
- )
+ assert result.returncode == 0, (
+ "package build failed "
+ f"(exit {result.returncode}):\n{result.stdout}\n{result.stderr}"
+ )As per coding guidelines, “Every active packaging, workflow, script, platform descriptor, Docker helper, and install/build document must be wired into the contract or removed; empty, stale, decorative, or unused packaging files are forbidden.”
🤖 Prompt for 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.
In `@tests/extensions/test_extensions_package_data_contract.py` around lines 102 -
106, The condition checking if result.returncode != 0 should not automatically
skip the test with pytest.skip() because this masks real build failures and
packaging regressions. Instead, distinguish between the build tool being
unavailable versus an actual build failure. Keep the skip only for cases where
the build tool itself is truly unavailable in the environment, but fail the test
hard (using assert or pytest.fail) when the build command actually runs but
returns a non-zero exit code, as this indicates a real packaging problem that
should not pass CI.
Source: Coding guidelines
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds contract coverage for the imported ECLI Extensions Layer assets under
src/ecli/extensions.Scope:
src/ecli/extensions/extensions/andEXTENSIONS_FOLDER-structure.mdzipfileandtarfileRepresentative checked assets:
src/ecli/extensions/cgmanifest.jsonsrc/ecli/extensions/bat/package.jsonsrc/ecli/extensions/bat/language-configuration.jsonsrc/ecli/extensions/bat/syntaxes/batchfile.tmLanguage.jsonsrc/ecli/extensions/bat/snippets/batchfile.code-snippetssrc/ecli/extensions/python/package.jsonsrc/ecli/extensions/python/language-configuration.jsonsrc/ecli/extensions/json/package.jsonsrc/ecli/extensions/javascript/package.jsonsrc/ecli/extensions/markdown-basics/package.jsonsrc/ecli/extensions/cpp/package.jsonConfirmed:
pyproject.tomlpackage-data change requiredValidation:
uv run ruff check src testsuv run ruff format --check src testsuv run python scripts/check_runtime_imports.pyuv run pytest -q tests/extensions tests/packaging tests/docs tests/ui/test_terminal_panel_reservation.py tests/ui/test_pysh_console_panel.py tests/ui/test_pysh_console_panel_keybindings.pyCloses #99.
Summary by CodeRabbit
Documentation
Tests