tests: Fix per-test teardown #6925
Conversation
📝 WalkthroughWalkthroughThis pull request updates the configuration and test structure. The constants file now defines a new Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test Method
participant S as swarm7 (session fixture)
participant R as swarm7_reset (function fixture)
T ->> S: Request swarm state
S -->> T: Provide swarm nodes
T ->> R: Invoke reset fixture before test
R -->> T: Reset swarm state
T ->> T: Execute test logic
Suggested labels
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🔇 Additional comments (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/test_win_prob.py (1)
300-300: Line exceeds maximum lengthThis line is slightly over the maximum length of 120 characters (122 > 120).
- # in this case, the relay has tickets for all the packets, because the source sends them with win prob = 1 + # in this case, the relay has tickets for all the packets, + # because the source sends them with win prob = 1🧰 Tools
🪛 Ruff (0.8.2)
300-300: Line too long (122 > 120)
(E501)
tests/test_websocket_api.py (1)
110-113: Good use of xfail for known issuesProperly documenting expected failures with
@pytest.mark.xfailis excellent practice, allowing tests to run while acknowledging known limitations.The xfail reason line is slightly over the maximum length (121 > 120).
- reason="This test is expected to fail due to a bug in the axum code, where the query is not parsed for the token" + reason="This test fails due to a bug in the axum code - query params aren't parsed for the token"🧰 Tools
🪛 Ruff (0.8.2)
111-111: Line too long (121 > 120)
(E501)
tests/test_integration.py (1)
337-347: Channel management test properly refactored.The test for opening and closing channels has been correctly converted to a class method.
The static analysis tool flagged line length issues at lines 341 and 345, but these are minor and don't affect functionality.
🧰 Tools
🪛 Ruff (0.8.2)
341-341: Line too long (123 > 120)
(E501)
345-345: Line too long (121 > 120)
(E501)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
sdk/python/localcluster/constants.py(2 hunks)tests/conftest.py(2 hunks)tests/pytest.ini(1 hunks)tests/test_hopli.py(9 hunks)tests/test_integration.py(1 hunks)tests/test_redeeming.py(1 hunks)tests/test_rest_api.py(1 hunks)tests/test_session.py(5 hunks)tests/test_websocket_api.py(1 hunks)tests/test_win_prob.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_websocket_api.py
111-111: Line too long (121 > 120)
(E501)
tests/test_win_prob.py
300-300: Line too long (122 > 120)
(E501)
tests/test_integration.py
341-341: Line too long (123 > 120)
(E501)
345-345: Line too long (121 > 120)
(E501)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: hoprd / docker
- GitHub Check: hopli / docker
- GitHub Check: Docs / Rust docs
- GitHub Check: tests-smoke-websocket
- GitHub Check: tests-unit-nightly
- GitHub Check: tests-unit
- GitHub Check: tests-smart-contracts
- GitHub Check: Linter
🔇 Additional comments (49)
tests/pytest.ini (1)
5-5: LGTM: Appropriate asyncio configuration for fixture optimizationSetting the asyncio fixture loop scope to "session" aligns perfectly with the changes made to the swarm7 fixture in conftest.py, ensuring consistent lifetimes between the asyncio loop and fixtures that depend on it.
sdk/python/localcluster/constants.py (3)
2-2: LGTM: Added necessary import for path handlingThe os module import is required for the path operations introduced in this file.
22-22: LGTM: Improved path handlingUsing os.path.join for constructing paths is a good practice for cross-platform compatibility.
29-29: LGTM: Standardized path constructionUsing the new CONTRACTS_DIR constant for path construction improves consistency and maintainability.
tests/conftest.py (3)
78-78: LGTM: Optimized fixture scopeChanging the swarm7 fixture scope from "module" to "session" should reduce test runtime when running multiple test modules, as stated in the PR objectives.
81-82: LGTM: Improved path handlingUsing os.path.join for constructing paths ensures better cross-platform compatibility.
90-103: LGTM: Well-implemented per-test teardown fixtureThe new swarm7_reset fixture properly implements the teardown functionality at function scope, with good error handling. This allows tests to run with a clean state while reusing the expensive swarm7 setup.
tests/test_hopli.py (3)
14-14: LGTM: Added necessary importAdding CONTRACTS_DIR to the imports supports the path standardization changes in this file.
63-63: LGTM: Standardized path referencesReplacing hardcoded strings with the CONTRACTS_DIR constant improves maintainability and consistency.
Also applies to: 91-91, 143-143, 219-219, 243-243, 291-291, 319-319, 339-339
347-489: LGTM: Well-structured test class implementationReorganizing tests into a class with the swarm7_reset fixture applied at the class level is a good approach. This:
- Makes the relationship between tests and fixtures more explicit
- Improves code organization
- Ensures proper teardown after each test function
- Maintains the original test logic while improving structure
tests/test_rest_api.py (1)
10-38: Well-organized refactoring to class-based testsThe reorganization of individual test functions into a class-based structure with
@pytest.mark.usefixtures("swarm7_reset")improves test organization and ensures each test runs with a clean state. The refactoring maintains the original test behavior while adding better isolation between tests.tests/test_win_prob.py (3)
6-6: Correctly updated import statementGood job updating the import statement to include
CONTRACTS_DIRfrom constants, which will be used to replace the hardcoded path in the function below.
33-33: Good use of CONTRACTS_DIR constantUsing the
CONTRACTS_DIRconstant instead of a hardcoded path is a best practice that improves maintainability and flexibility.
42-124: Well-structured test with proper cleanupThe test functions appropriately handle setting and restoring the winning probability in a try-finally block, ensuring cleanup even if the test fails. This is good defensive programming.
tests/test_websocket_api.py (4)
26-44: Good implementation of auth validation testsThis test correctly verifies that WebSocket connections are rejected when authentication tokens are missing.
45-61: Comprehensive authentication testingThe test properly verifies that connections with invalid tokens are rejected with a 401 status code.
62-78: Thorough testing of authentication edge casesTesting query parameter-based authentication is important for API security. Good job including this test.
79-95: Complete Bearer token authentication testingAdding tests for Bearer token authentication ensures all authentication methods are properly verified.
tests/test_session.py (2)
234-236: Good class structure with proper fixture usageUsing
swarm7_resetfixture at the class level ensures all tests have a clean environment.
255-255: Path parameter structure simplifiedThe path parameter has been simplified from a more complex structure to
{"Hops": 0}, which appears to be a cleaner approach.tests/test_redeeming.py (7)
27-28: Well-structured class refactoring with appropriate fixture usage.The introduction of the
TestRedeemingWithSwarmclass with theswarm7_resetfixture ensures proper state cleanup between test runs while maintaining the session-scopedswarm7fixture, aligning with the PR objective of reducing overall test runtime.
31-36: LGTM! Test method correctly refactored.The standalone function has been properly converted to a class method with the
selfparameter added, without changing the test logic.
42-72: Test refactoring maintains original functionality.The test has been correctly refactored into a class method with proper indentation and parameter adjustments, while preserving the original test logic and assertions.
77-119: Complex test successfully refactored to class method.This test case has been properly converted to a class method with the correct
selfparameter. The test logic, including channel creation, packet sending, and assertion checking remains unchanged.
125-156: Ticket aggregation and redemption test properly refactored.The method has been correctly refactored with consistent indentation and parameter adjustments while maintaining all the original test functionality.
163-197: Multi-hop routing test correctly refactored.The test for redeemable tickets in n-hop routing has been properly converted to a class method with appropriate parameter updates and indentation adjustments.
200-227: Channel closing test successfully refactored.The final test case has been properly refactored into a class method with correct indentation and parameter updates, preserving the original test logic.
tests/test_integration.py (22)
50-82: Well-structured class organization for integration tests.The implementation of
TestIntegrationWithSwarmwith theswarm7_resetfixture matches the PR objective of making tests use session-scoped fixtures with per-test resets. The connectivity test has been properly refactored into a class method.
83-91: Balance check test correctly refactored.The test for protocol balance checking has been properly converted to a class method with the required
selfparameter.
92-112: Alias management test successfully refactored.The test for alias removal has been correctly refactored as a class method while maintaining the original assertion logic.
113-127: Multiple aliases test properly converted to class method.This test has been correctly refactored with proper indentation and parameter adjustments.
128-132: Self alias test properly refactored.The test for automatic self alias has been correctly converted to a class method.
133-142: Ping functionality test correctly refactored.The test for ping functionality between nodes has been properly refactored into a class method.
143-149: Self-ping test successfully refactored.The test for ping to self has been properly converted to a class method.
150-165: Unfinished ping test correctly refactored.The unfinished test for pinging non-registered nodes has been properly refactored into a class method, maintaining the same placeholder implementation.
166-179: Zero-hop message test properly refactored.This test for sending messages without open channels has been correctly converted to a class method.
180-191: Large message test correctly refactored.The test for large message sending has been properly converted to a class method.
192-224: Channel funding test successfully refactored.The test for channel funding has been properly converted to a class method while maintaining the original assertion logic.
229-251: Ticket statistics reset test properly refactored.This test for resetting ticket statistics has been correctly refactored as a class method.
256-334: Channel funding depletion test correctly refactored.This test for message sending with depleted funding has been properly converted to a class method. The extensive commented code remains as documentation for future implementation.
361-396: Strategy test successfully refactored.The test for default strategy and ticket aggregation has been properly converted to a class method while maintaining the original logic.
398-414: Channel status test correctly refactored.The sanity check for channel status has been properly refactored as a class method.
415-433: Native withdraw test properly refactored.The test for native token withdrawal has been correctly converted to a class method.
434-441: Ticket price test successfully refactored.The test for checking default ticket price has been properly converted to a class method.
442-452: Reserved tag test correctly refactored.The test for sending messages with reserved application tags has been properly converted to a class method.
453-462: Inbox operations test properly refactored.The test for inbox operations with reserved tags has been correctly refactored as a class method.
463-505: Message peeking test correctly refactored.The test for peeking messages with timestamp has been properly converted to a class method while preserving the original logic.
506-526: Message timestamp test properly refactored.The test for message send timestamp has been correctly converted to a class method.
527-545: Message addressing test successfully refactored.The test for sending messages with address or peer ID has been properly converted to a class method.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
sdk/python/localcluster/constants.py (1)
2-2: Remove unused import.The
osmodule is imported but not used anywhere in the file.- import os🧰 Tools
🪛 Ruff (0.8.2)
2-2:
osimported but unusedRemove unused import:
os(F401)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sdk/python/localcluster/constants.py(2 hunks)tests/conftest.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/conftest.py
🧰 Additional context used
🪛 Ruff (0.8.2)
sdk/python/localcluster/constants.py
2-2: os imported but unused
Remove unused import: os
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: hoprd / docker
- GitHub Check: hopli / docker
- GitHub Check: tests-unit-nightly
- GitHub Check: Docs / Rust docs
- GitHub Check: tests-unit
- GitHub Check: tests-smoke-websocket
- GitHub Check: tests-smart-contracts
- GitHub Check: Linter
🔇 Additional comments (2)
sdk/python/localcluster/constants.py (2)
22-22: Consider defining CONTRACTS_DIR more explicitly as a Path object.The current implementation uses relative path traversal with
../../../, which can be error-prone and harder to maintain. While this approach does make the path relative to the file location rather than the working directory, it would be more readable to define it explicitly.For context: The previous discussion between reviewers about using
PWDvs. file-relative paths wasn't fully resolved. The advantage of the current approach is that it makes the code independent of the working directory, but the multiple parent directory traversals (../../../) could be more clearly expressed.
29-29: Looks good - consistent with the new CONTRACTS_DIR constant.This change properly leverages the newly defined
CONTRACTS_DIRconstant to locate the contracts-addresses.json file, making the path references more consistent throughout the codebase.
(cherry picked from commit 7b4ca6c)
(cherry picked from commit 7b4ca6c)
This change is needed to add alternative swarm7 fixture implementations.
Refs #6908