Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive test coverage for the krkn/cerberus/setup.py module, improving code coverage from the current 98% to potentially 100%. The tests focus on three main functions: get_status, publish_kraken_status, and application_status, which handle Cerberus health monitoring and status reporting.
- Adds 338 lines of unit tests covering various scenarios for Cerberus integration
- Creates test package structure with proper initialization
- Tests cover both healthy and unhealthy states, error conditions, and edge cases
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/cerberus/test_setup.py | New comprehensive test suite for cerberus setup module with 3 test classes covering all major functions and edge cases |
| tests/cerberus/init.py | New package initialization file for cerberus test module |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/cerberus/test_setup.py
Outdated
| "cerberus": { | ||
| "cerberus_enabled": True, | ||
| "cerberus_url": "http://cerberus:8080", | ||
| "check_applicaton_routes": False |
There was a problem hiding this comment.
Spelling error: "applicaton" should be "application". This typo is consistent with the actual implementation code (line 17 in setup.py uses "check_applicaton_routes"), but it's still incorrect spelling.
tests/cerberus/test_setup.py
Outdated
| "cerberus": { | ||
| "cerberus_enabled": True, | ||
| "cerberus_url": "http://cerberus:8080", | ||
| "check_applicaton_routes": True |
There was a problem hiding this comment.
Spelling error: "applicaton" should be "application". This typo is consistent with the actual implementation code (line 17 in setup.py uses "check_applicaton_routes"), but it's still incorrect spelling.
|
@iting0321 Thank you very much for your contributions, could you please solve the issues identified by copilot? Thanks a lot! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "cerberus": { | ||
| "cerberus_enabled": True, | ||
| "cerberus_url": "http://cerberus:8080", | ||
| "check_application_routes": True |
There was a problem hiding this comment.
The config dictionary uses check_application_routes but the actual source code uses check_applicaton_routes (with a typo - missing 'i'). This test will not accurately test the code path that checks application routes since the key doesn't match what the implementation looks for.
tests/cerberus/test_setup.py
Outdated
| application_status("http://cerberus:8080", 0, 6000) | ||
|
|
||
| mock_exit.assert_called_once_with(1) |
There was a problem hiding this comment.
This test doesn't properly verify the behavior when sys.exit is called. The test should raise SystemExit but doesn't set up the mock to do so. Without mock_exit.side_effect = SystemExit(1), the test won't raise SystemExit and will just continue execution, making the assertion less meaningful.
tests/cerberus/test_setup.py
Outdated
| application_status("http://cerberus:8080", 0, 6000) | ||
|
|
||
| mock_exit.assert_called_once_with(1) |
There was a problem hiding this comment.
This test doesn't properly verify the behavior when sys.exit is called. The test should raise SystemExit but doesn't set up the mock to do so. Without mock_exit.side_effect = SystemExit(1), the test won't raise SystemExit and will just continue execution, making the assertion less meaningful.
| with patch('krkn.cerberus.setup.sys.exit') as mock_exit: | ||
| publish_kraken_status(config, ["failed_scenario"], 0, 100) | ||
| mock_exit.assert_called_once_with(1) |
There was a problem hiding this comment.
This test doesn't properly verify the sys.exit behavior. Without setting mock_exit.side_effect = SystemExit(1), the mock will prevent the actual SystemExit from being raised, making the test less meaningful. The test should either verify that exit was called or properly simulate the SystemExit exception.
| with patch('krkn.cerberus.setup.sys.exit') as mock_exit: | ||
| publish_kraken_status(config, ["failed_scenario"], 0, 100) | ||
| mock_exit.assert_called_once_with(1) |
There was a problem hiding this comment.
This test doesn't properly verify the sys.exit behavior. Without setting mock_exit.side_effect = SystemExit(1), the mock will prevent the actual SystemExit from being raised, making the test less meaningful. The test should either verify that exit was called or properly simulate the SystemExit exception.
| "cerberus": { | ||
| "cerberus_enabled": True, | ||
| "cerberus_url": "http://cerberus:8080", | ||
| "check_application_routes": False |
There was a problem hiding this comment.
The config dictionary uses check_application_routes but the actual source code uses check_applicaton_routes (with a typo - missing 'i'). This test will not accurately test the code path that checks application routes since the key doesn't match what the implementation looks for.
| "cerberus": { | ||
| "cerberus_enabled": True, | ||
| "cerberus_url": "http://cerberus:8080", | ||
| "check_application_routes": True |
There was a problem hiding this comment.
The config dictionary uses check_application_routes but the actual source code uses check_applicaton_routes (with a typo - missing 'i'). This test will not accurately test the code path that checks application routes since the key doesn't match what the implementation looks for.
tests/cerberus/test_setup.py
Outdated
| get_status(config, 0, 100) | ||
|
|
||
| mock_exit.assert_called_once_with(1) |
There was a problem hiding this comment.
This test doesn't properly verify the behavior when sys.exit is called. The test should raise SystemExit but doesn't set up the mock to do so. Without mock_exit.side_effect = SystemExit(1), the test won't raise SystemExit and will just continue execution, making the assertion meaningless.
tests/cerberus/test_setup.py
Outdated
| get_status(config, 0, 100) | ||
|
|
||
| mock_exit.assert_called_once_with(1) |
There was a problem hiding this comment.
This test doesn't properly verify the behavior when sys.exit is called. The test should raise SystemExit but doesn't set up the mock to do so. Without mock_exit.side_effect = SystemExit(1), the test won't raise SystemExit and will just continue execution, making the assertion less meaningful.
| "cerberus": { | ||
| "cerberus_enabled": True, | ||
| "cerberus_url": "http://cerberus:8080", | ||
| "check_application_routes": False |
There was a problem hiding this comment.
The config dictionary uses check_application_routes but the actual source code uses check_applicaton_routes (with a typo - missing 'i'). This test will not accurately test the code path that checks application routes since the key doesn't match what the implementation looks for.
|
Because “application” was misspelled as “applicaton” at line 17 in |
…application routes
Type of change
Description
Related Tickets & Documents
Documentation
If checked, a documentation PR must be created and merged in the website repository.
Related Documentation PR (if applicable)
Checklist before requesting a review