Refactor workspace metadata test setup - #12
Conversation
Reviewer's GuideCentralize workspace metadata test setup by introducing reusable manifest and package helper functions and refactor the workspace graph construction test to leverage these utilities for cleaner, more maintainable tests. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughAdds a test helper module that creates Cargo.toml manifests and deterministic package metadata, and updates a unit test to use these helpers instead of manually constructing manifests and package dictionaries. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as tests/unit/test_workspace_metadata.py
participant Helpers as tests/helpers/workspace_metadata.py
participant FS as Filesystem
Note over Test,Helpers: Test delegates manifest & package construction to helpers
Test->>Helpers: _create_test_manifest(workspace_root, crate_name, content)
Helpers->>FS: write workspace_root/crate_name/Cargo.toml
FS-->>Helpers: manifest_path
Test->>Helpers: _build_test_package(name, version, manifest_path, **kwargs)
Helpers-->>Test: package_metadata_dict
Test->>Test: run assertions comparing workspace metadata to expected dicts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider extracting the new _create_test_manifest and _build_test_package helpers into a shared test-utils module so they can be reused across multiple tests.
- Normalize the "publish" field in _build_test_package to always be a list (for example default to []) to avoid mixing None and list types in your test payloads.
- Rename _build_test_package to something like _create_test_package_metadata to make it clearer that it returns a metadata dict rather than building a full Package object.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the new _create_test_manifest and _build_test_package helpers into a shared test-utils module so they can be reused across multiple tests.
- Normalize the "publish" field in _build_test_package to always be a list (for example default to []) to avoid mixing None and list types in your test payloads.
- Rename _build_test_package to something like _create_test_package_metadata to make it clearer that it returns a metadata dict rather than building a full Package object.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/unit/test_workspace_metadata.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/*.py: Python files must use snake_case filenames (e.g., http_client.py, task_queue.py)
Classes must use PascalCase
Variables and functions must use snake_case
Module-level constants must use UPPER_SNAKE_CASE
Prefix non-exported helpers or internal APIs with a single leading underscore
Use typing everywhere and maintain full static type coverage
Use TypedDict or @DataClass for structured data; prefer @DataClass(slots=True) for internal-only usage
Avoid Any; prefer precise types (TypeVar, Protocol, Literal, Union); use typing.cast only when necessary with justification; use object for unknown opaque values
Be explicit with return types (e.g., -> None, -> str) for all public functions and methods
Favor immutability: prefer tuples to lists and MappingProxyType for read-only mappings; document third-party frozendict if used
Use# pyright: ignoresparingly and include an explanation comment when used
Avoid side effects at import time (no global state mutation or actions on import)
Use docstrings (NumPy format) for public functions, classes, and modules
Explain tricky code with inline comments
**/*.py: Prefer context managers to encapsulate setup/teardown for resources (files, locks, connections) instead of manual try/finally blocks
Use contextlib.contextmanager (@contextmanager) to implement simple, linear setup/teardown context managers
Implement a class with enter and exit for context managers that require internal state or more complex lifecycle handling
For file I/O, prefer with open(...) as ... over open()/try/finally/close patterns
Choose @contextmanager when control flow is linear and no persistent state is needed
Choose a class-based context manager when there is internal state, lifecycle methods, re-entry, or advanced context needs
**/*.py: Exception classes must end with the suffix 'Error' (N818)
Prefer specific built-ins (e.g., TypeError, ValueError) or domain exceptions over raising Exception directly (TRY003/TRY004)
Preserve causal chains...
Files:
tests/unit/test_workspace_metadata.py
{**/unittests/test_*.py,tests/**/*.py}
📄 CodeRabbit inference engine (.rules/python-00.md)
{**/unittests/test_*.py,tests/**/*.py}: Use pytest idioms: prefer fixtures over setup/teardown, parametrize broadly, avoid unnecessary mocks
Group related tests using classes with method names prefixed by test_
Write tests from a user's perspective: test public behaviour, not internals
Files:
tests/unit/test_workspace_metadata.py
tests/**/*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
In tests, assert specific exception types (and optional message via regex) rather than using broad Exception (B017)
Files:
tests/unit/test_workspace_metadata.py
🪛 GitHub Actions: CI
tests/unit/test_workspace_metadata.py
[error] 187-187: D202: No blank lines allowed after function docstring. Remove blank line(s) after the docstring. 2 fixable with the --fix option.
[error] 203-203: D202: No blank lines allowed after function docstring. Remove blank line(s) after the docstring. 2 fixable with the --fix option.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (1)
tests/unit/test_workspace_metadata.py (1)
218-260: Excellent refactoring: test setup is now more maintainable.The extraction of manifest creation and package metadata construction into dedicated helpers significantly improves readability and reduces duplication. The test logic and assertions remain unchanged, ensuring behavioral equivalence.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/unit/test_workspace_metadata.py Comment on lines +196 to +212 def _build_test_package(
name: str,
version: str,
manifest_path: Path,
dependencies: list[dict[str, str]] | None = None,
publish: list[str] | None = None,
) -> dict[str, typ.Any]:
"""Create package metadata with predictable identifiers for tests."""
return {
"name": name,
"version": version,
"id": f"{name}-id",
"manifest_path": str(manifest_path),
"dependencies": dependencies if dependencies is not None else [],
"publish": publish,
}❌ New issue: Excess Number of Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/helpers/workspace_metadata.py(1 hunks)tests/unit/test_workspace_metadata.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/*.py: Python files must use snake_case filenames (e.g., http_client.py, task_queue.py)
Classes must use PascalCase
Variables and functions must use snake_case
Module-level constants must use UPPER_SNAKE_CASE
Prefix non-exported helpers or internal APIs with a single leading underscore
Use typing everywhere and maintain full static type coverage
Use TypedDict or @DataClass for structured data; prefer @DataClass(slots=True) for internal-only usage
Avoid Any; prefer precise types (TypeVar, Protocol, Literal, Union); use typing.cast only when necessary with justification; use object for unknown opaque values
Be explicit with return types (e.g., -> None, -> str) for all public functions and methods
Favor immutability: prefer tuples to lists and MappingProxyType for read-only mappings; document third-party frozendict if used
Use# pyright: ignoresparingly and include an explanation comment when used
Avoid side effects at import time (no global state mutation or actions on import)
Use docstrings (NumPy format) for public functions, classes, and modules
Explain tricky code with inline comments
**/*.py: Prefer context managers to encapsulate setup/teardown for resources (files, locks, connections) instead of manual try/finally blocks
Use contextlib.contextmanager (@contextmanager) to implement simple, linear setup/teardown context managers
Implement a class with enter and exit for context managers that require internal state or more complex lifecycle handling
For file I/O, prefer with open(...) as ... over open()/try/finally/close patterns
Choose @contextmanager when control flow is linear and no persistent state is needed
Choose a class-based context manager when there is internal state, lifecycle methods, re-entry, or advanced context needs
**/*.py: Exception classes must end with the suffix 'Error' (N818)
Prefer specific built-ins (e.g., TypeError, ValueError) or domain exceptions over raising Exception directly (TRY003/TRY004)
Preserve causal chains...
Files:
tests/helpers/workspace_metadata.pytests/unit/test_workspace_metadata.py
{**/unittests/test_*.py,tests/**/*.py}
📄 CodeRabbit inference engine (.rules/python-00.md)
{**/unittests/test_*.py,tests/**/*.py}: Use pytest idioms: prefer fixtures over setup/teardown, parametrize broadly, avoid unnecessary mocks
Group related tests using classes with method names prefixed by test_
Write tests from a user's perspective: test public behaviour, not internals
Files:
tests/helpers/workspace_metadata.pytests/unit/test_workspace_metadata.py
tests/**/*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
In tests, assert specific exception types (and optional message via regex) rather than using broad Exception (B017)
Files:
tests/helpers/workspace_metadata.pytests/unit/test_workspace_metadata.py
🧬 Code graph analysis (2)
tests/helpers/workspace_metadata.py (1)
crate_tools/unittests/test_publish_workspace_dependencies.py (1)
workspace_root(16-21)
tests/unit/test_workspace_metadata.py (1)
tests/helpers/workspace_metadata.py (2)
_build_test_package(20-42)_create_test_manifest(10-17)
🪛 GitHub Actions: CI
tests/helpers/workspace_metadata.py
[error] 7-7: TC003 Move standard library import pathlib.Path into a type-checking block
[error] 11-11: D202: No blank lines allowed after function docstring
[error] 24-24: ANN401 Dynamically typed expressions (typing.Any) are disallowed in **kwargs
[error] 26-26: D202: No blank lines allowed after function docstring
[error] 28-28: D413 Missing blank line after last section ("Args")
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (2)
tests/unit/test_workspace_metadata.py (2)
24-24: LGTM!The import of the new test helpers is correct and aligns with the refactoring objectives.
190-231: LGTM!The refactoring successfully replaces manual manifest and package metadata construction with the shared helper functions. The test logic and assertions are preserved, and the helper usage is correct.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/helpers/workspace_metadata.py Comment on lines +20 to +38 def _build_test_package(
name: str,
version: str,
manifest_path: Path,
**kwargs: typ.Any,
) -> dict[str, typ.Any]:
"""Create package metadata with predictable identifiers for tests.
Args:
name: Package name
version: Package version
manifest_path: Path to the manifest file
**kwargs: Optional fields (dependencies, publish, etc.)
"""
return {
"name": name,
"version": version,
"id": f"{name}-id",❌ New issue: Excess Number of Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/helpers/workspace_metadata.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/*.py: Python files must use snake_case filenames (e.g., http_client.py, task_queue.py)
Classes must use PascalCase
Variables and functions must use snake_case
Module-level constants must use UPPER_SNAKE_CASE
Prefix non-exported helpers or internal APIs with a single leading underscore
Use typing everywhere and maintain full static type coverage
Use TypedDict or @DataClass for structured data; prefer @DataClass(slots=True) for internal-only usage
Avoid Any; prefer precise types (TypeVar, Protocol, Literal, Union); use typing.cast only when necessary with justification; use object for unknown opaque values
Be explicit with return types (e.g., -> None, -> str) for all public functions and methods
Favor immutability: prefer tuples to lists and MappingProxyType for read-only mappings; document third-party frozendict if used
Use# pyright: ignoresparingly and include an explanation comment when used
Avoid side effects at import time (no global state mutation or actions on import)
Use docstrings (NumPy format) for public functions, classes, and modules
Explain tricky code with inline comments
**/*.py: Prefer context managers to encapsulate setup/teardown for resources (files, locks, connections) instead of manual try/finally blocks
Use contextlib.contextmanager (@contextmanager) to implement simple, linear setup/teardown context managers
Implement a class with enter and exit for context managers that require internal state or more complex lifecycle handling
For file I/O, prefer with open(...) as ... over open()/try/finally/close patterns
Choose @contextmanager when control flow is linear and no persistent state is needed
Choose a class-based context manager when there is internal state, lifecycle methods, re-entry, or advanced context needs
**/*.py: Exception classes must end with the suffix 'Error' (N818)
Prefer specific built-ins (e.g., TypeError, ValueError) or domain exceptions over raising Exception directly (TRY003/TRY004)
Preserve causal chains...
Files:
tests/helpers/workspace_metadata.py
{**/unittests/test_*.py,tests/**/*.py}
📄 CodeRabbit inference engine (.rules/python-00.md)
{**/unittests/test_*.py,tests/**/*.py}: Use pytest idioms: prefer fixtures over setup/teardown, parametrize broadly, avoid unnecessary mocks
Group related tests using classes with method names prefixed by test_
Write tests from a user's perspective: test public behaviour, not internals
Files:
tests/helpers/workspace_metadata.py
tests/**/*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
In tests, assert specific exception types (and optional message via regex) rather than using broad Exception (B017)
Files:
tests/helpers/workspace_metadata.py
🧬 Code graph analysis (1)
tests/helpers/workspace_metadata.py (1)
crate_tools/unittests/test_publish_workspace_dependencies.py (1)
workspace_root(16-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (1)
tests/helpers/workspace_metadata.py (1)
21-43: LGTM! The**kwargsapproach addresses the CodeScene warning.The refactoring to use
**kwargsreduces the formal parameter count from 5 to 4, satisfying CodeScene's "Excess Number of Function Arguments" rule while preserving the function's behavior. The implementation correctly extracts optional fields with sensible defaults.Based on PR objectives.
|
@coderabbitai pause |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/helpers/workspace_metadata.py (2)
5-5: Remove underscore prefix from exported function names.Python convention reserves underscore-prefixed names for private/internal APIs, yet both functions are explicitly exported in
__all__. This creates a contradiction: the prefix signals "private" while the export declares them public.Either rename the functions to remove the prefix:
-__all__ = ["_build_test_package", "_create_test_manifest"] +__all__ = ["build_test_package", "create_test_manifest"]and update the function definitions accordingly (lines 14, 23), or remove them from
__all__if they should remain internal.
23-45: LGTM! Consider TypedDict for stronger typing.The function correctly addresses the "excess arguments" issue flagged in PR objectives by consolidating optional parameters into
**kwargs: object. All past lint issues (ANN401, D202, D413) are resolved.For stronger type safety, you could define a TypedDict for kwargs at the module level:
from typing import TypedDict class PackageKwargs(TypedDict, total=False): dependencies: list[dict[str, typ.Any]] publish: bool | NoneThen annotate:
**kwargs: typ.Unpack[PackageKwargs](Python 3.12+) or keepobjectif backward compatibility is needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/helpers/workspace_metadata.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/*.py: Python files must use snake_case filenames (e.g., http_client.py, task_queue.py)
Classes must use PascalCase
Variables and functions must use snake_case
Module-level constants must use UPPER_SNAKE_CASE
Prefix non-exported helpers or internal APIs with a single leading underscore
Use typing everywhere and maintain full static type coverage
Use TypedDict or @DataClass for structured data; prefer @DataClass(slots=True) for internal-only usage
Avoid Any; prefer precise types (TypeVar, Protocol, Literal, Union); use typing.cast only when necessary with justification; use object for unknown opaque values
Be explicit with return types (e.g., -> None, -> str) for all public functions and methods
Favor immutability: prefer tuples to lists and MappingProxyType for read-only mappings; document third-party frozendict if used
Use# pyright: ignoresparingly and include an explanation comment when used
Avoid side effects at import time (no global state mutation or actions on import)
Use docstrings (NumPy format) for public functions, classes, and modules
Explain tricky code with inline comments
**/*.py: Prefer context managers to encapsulate setup/teardown for resources (files, locks, connections) instead of manual try/finally blocks
Use contextlib.contextmanager (@contextmanager) to implement simple, linear setup/teardown context managers
Implement a class with enter and exit for context managers that require internal state or more complex lifecycle handling
For file I/O, prefer with open(...) as ... over open()/try/finally/close patterns
Choose @contextmanager when control flow is linear and no persistent state is needed
Choose a class-based context manager when there is internal state, lifecycle methods, re-entry, or advanced context needs
**/*.py: Exception classes must end with the suffix 'Error' (N818)
Prefer specific built-ins (e.g., TypeError, ValueError) or domain exceptions over raising Exception directly (TRY003/TRY004)
Preserve causal chains...
Files:
tests/helpers/workspace_metadata.py
{**/unittests/test_*.py,tests/**/*.py}
📄 CodeRabbit inference engine (.rules/python-00.md)
{**/unittests/test_*.py,tests/**/*.py}: Use pytest idioms: prefer fixtures over setup/teardown, parametrize broadly, avoid unnecessary mocks
Group related tests using classes with method names prefixed by test_
Write tests from a user's perspective: test public behaviour, not internals
Files:
tests/helpers/workspace_metadata.py
tests/**/*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
In tests, assert specific exception types (and optional message via regex) rather than using broad Exception (B017)
Files:
tests/helpers/workspace_metadata.py
🧬 Code graph analysis (1)
tests/helpers/workspace_metadata.py (1)
crate_tools/unittests/test_publish_workspace_dependencies.py (1)
workspace_root(16-21)
🔇 Additional comments (1)
tests/helpers/workspace_metadata.py (1)
14-20: LGTM!The helper correctly creates a manifest directory structure and writes normalized content. The TYPE_CHECKING import of
Pathworks correctly becausefrom __future__ import annotationsdefers annotation evaluation, and allPathoperations are on the instance passed as a parameter.
✅ Actions performedReviews paused. |
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68f0398f0bc483229beafb66abb0eb00
Summary by Sourcery
Refactor workspace metadata tests by extracting common manifest and package creation logic into helper functions and applying them in the workspace graph construction test for improved readability and maintainability.
Enhancements:
Tests:
Summary by CodeRabbit
Note: Internal test tooling improvements only — no changes to user-facing functionality.