Enable Controller UT #9
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe transfer queue controller API is simplified by removing the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes involve straightforward API refactoring across multiple files with consistent patterns. Complexity stems from coordinating constructor signature changes, method removals, and corresponding test updates. The docstring reformatting is trivial but contributes to overall file variety. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR removes unused functionality related to storage unit management and index mapping from the TransferQueueController, simplifying the codebase. The changes appear to be part of enabling controller unit tests by removing dependencies on storage units.
Key changes:
- Removed the
num_storage_unitsparameter from TransferQueueController initialization - Deleted the
get_global_index_mappingmethod and its related test - Removed fixture dependencies on storage unit registration
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| transfer_queue/controller.py | Removed num_storage_units docstring parameter and deleted the get_global_index_mapping method |
| transfer_queue/client.py | Reformatted docstring for better readability (line length) |
| tests/test_controller.py | Removed unused imports, deleted storage-related fixtures and tests, updated test to use simplified fixture |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_controller.py (1)
97-129: Reduced test coverage for index-storage mapping.According to the AI summary, assertions for
local_indexesandstorage_idswere removed from this test. The test now only verifiesglobal_indexesafter a reorder operation. If the removed assertions were testing important controller behavior related to how global indexes map to local storage indexes and storage unit IDs, this represents a significant reduction in test coverage.Consider whether these assertions should be restored or if separate tests are needed to verify:
- Local index assignment correctness
- Storage unit ID mapping accuracy
- The relationship between global, local, and storage indexes
🧹 Nitpick comments (1)
tests/test_controller.py (1)
26-26: Verify import consistency in tests/test_controller.py.
TransferQueueControlleris correctly exported fromtransfer_queue/__init__.py, so the import path change is valid. However, the test file has mixed import sources:TransferQueueControllerfrom the package root (line 26) whileTQ_INIT_FIELD_NUMremains imported fromtransfer_queue.controller(line 27). For consistency, both should be imported from the same location—either both from the package root or both from the submodule.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/test_controller.py(2 hunks)transfer_queue/client.py(1 hunks)transfer_queue/controller.py(0 hunks)
💤 Files with no reviewable changes (1)
- transfer_queue/controller.py
🧰 Additional context used
🪛 GitHub Actions: pre-commit
tests/test_controller.py
[error] 26-26: ruff: E402 Module level import not at top of file.
🪛 GitHub Check: pre-commit (3.10)
tests/test_controller.py
[failure] 27-27: Ruff (E402)
tests/test_controller.py:27:1: E402 Module level import not at top of file
[failure] 26-26: Ruff (E402)
tests/test_controller.py:26:1: E402 Module level import not at top of file
🔇 Additional comments (1)
transfer_queue/client.py (1)
64-65: LGTM!The docstring formatting improvement enhances readability without changing functionality.
tests/test_controller.py
Outdated
| from transfer_queue import TransferQueueController | ||
| from transfer_queue.controller import TQ_INIT_FIELD_NUM |
There was a problem hiding this comment.
Fix import order to resolve pipeline failure.
The pipeline is failing due to E402 violations - module-level imports must appear before the sys.path manipulation on lines 23-24. This violates PEP 8 conventions.
Consider one of these solutions:
Solution 1 (Recommended): Suppress the rule if sys.path manipulation is required
Add a # noqa: E402 comment after each import:
parent_dir = Path(__file__).resolve().parent.parent
sys.path.append(str(parent_dir))
-from transfer_queue import TransferQueueController
-from transfer_queue.controller import TQ_INIT_FIELD_NUM
+from transfer_queue import TransferQueueController # noqa: E402
+from transfer_queue.controller import TQ_INIT_FIELD_NUM # noqa: E402Solution 2 (Better long-term): Ensure proper package installation
Configure the test environment to install the package properly so sys.path manipulation isn't needed, then move imports to the top of the file.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from transfer_queue import TransferQueueController | |
| from transfer_queue.controller import TQ_INIT_FIELD_NUM | |
| from transfer_queue import TransferQueueController # noqa: E402 | |
| from transfer_queue.controller import TQ_INIT_FIELD_NUM # noqa: E402 |
🧰 Tools
🪛 GitHub Actions: pre-commit
[error] 26-26: ruff: E402 Module level import not at top of file.
🪛 GitHub Check: pre-commit (3.10)
[failure] 27-27: Ruff (E402)
tests/test_controller.py:27:1: E402 Module level import not at top of file
[failure] 26-26: Ruff (E402)
tests/test_controller.py:26:1: E402 Module level import not at top of file
🤖 Prompt for AI Agents
In tests/test_controller.py around lines 26 to 27, the two module-level imports
are placed after sys.path manipulation causing E402 failures; fix by either (a)
adding a "# noqa: E402" comment to each of these import lines to suppress the
E402 rule if you must keep sys.path tweaks, or (b) preferably move these imports
above the sys.path modification and ensure the test environment installs the
package so sys.path manipulation is unnecessary.
…to dev_lxm_controller_ut
0oshowero0
merged commit a9d5bbf
into
0oshowero0:han/unified_storage_abstract
simply msg body
2 participants
Summary by CodeRabbit
Refactor
Tests