Conversation
…mproved coverage.
|
I have read the CLA Document and I hereby sign the CLA 2 out of 3 committers have signed the CLA. |
There was a problem hiding this comment.
Pull request overview
Adds a consolidated test module and updates the test specification docs to reflect alias-first operation in XTS.
Changes:
- Added
test/test_xts_all_cases.pyto cover alias management and allocator client CLI flows. - Updated
docs/test_documentation.mdto describe alias-based execution semantics and expected behaviors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| test/test_xts_all_cases.py | New aggregated tests for alias management + allocator client commands and error handling scenarios. |
| docs/test_documentation.md | Updated test specification language to emphasize alias-only operation and related expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_allocator_add_server(mock_client): | ||
| args = ['allocator', 'add', 'test_allocator', 'http://allocator-server'] | ||
| with patch('sys.stdout', new_callable=StringIO) as mock_stdout: | ||
| try: | ||
| mock_client.run(args) | ||
| except SystemExit: | ||
| pass | ||
| output = mock_stdout.getvalue() | ||
| assert ("Server added" in output or "Usage:" in output or "error" in output.lower()) | ||
|
|
||
| def test_allocator_remove_server(mock_client): | ||
| args = ['allocator', 'remove', 'test_allocator'] | ||
| with patch('sys.stdout', new_callable=StringIO) as mock_stdout: | ||
| try: | ||
| mock_client.run(args) | ||
| except SystemExit: | ||
| pass | ||
| output = mock_stdout.getvalue() | ||
| assert ("Server removed" in output or "Usage:" in output or "error" in output.lower()) |
There was a problem hiding this comment.
mock_client patches _send_request but does not isolate XTSAllocatorClient.CONFIG_FILE. Tests like allocator add/remove will write to the real ~/.xts_servers.yaml (path is computed at import time from Path.home()). Patch XTSAllocatorClient.CONFIG_FILE to a tmp file (as done in test_xts_allocator_client_positive_mocks.py) to avoid polluting user config and to keep tests hermetic.
| def test_missing_alias(monkeypatch, mock_alias_config): | ||
| """Test running XTS with a missing alias.""" | ||
| # Remove all aliases before test | ||
| aliases = load_aliases() | ||
| for alias in list(aliases.keys()): | ||
| remove_alias(alias) | ||
| with patch('sys.stdout', new_callable=StringIO) as mock_stdout: | ||
| result = load_aliases() | ||
| assert result == {} # No aliases present | ||
| output = mock_stdout.getvalue() | ||
| # Optionally check for error message if CLI is invoked | ||
|
|
There was a problem hiding this comment.
test_missing_alias currently only clears the aliases file and asserts load_aliases() == {}; it doesn't actually test the user-visible behavior of running xts with an unknown alias (which should exit non-zero and print Unknown alias ...). Consider invoking xts_core.xts.XTS()._parse_first_arg() with sys.argv patched, or calling the top-level main()/run() entrypoint and asserting on exit code + message.
| - Execution of test cases should not proceed. | ||
| - AllocatorClient commands should remain functional and usable. |
There was a problem hiding this comment.
This statement seems inconsistent with the current CLI flow: xts requires the first argument to be --alias or a valid alias name, so when no alias is registered you cannot reach allocator subcommands via xts (it exits early with Unknown alias). If the allocator client is intended to be usable without an alias, the docs should clarify the standalone invocation path; otherwise, adjust this expectation.
| - Execution of test cases should not proceed. | |
| - AllocatorClient commands should remain functional and usable. | |
| - Execution of test cases and allocator-related subcommands via `xts` should not proceed. | |
| - This scenario validates correct error handling in alias-only mode; no allocator commands are expected to run when invoked through `xts` without a valid alias. |
| @pytest.fixture | ||
| def mock_alias_config(monkeypatch, tmp_path): | ||
| temp_home = tmp_path / "home" | ||
| config_dir = temp_home / ".xts" | ||
| config_dir.mkdir(parents=True, exist_ok=True) | ||
| monkeypatch.setenv("HOME", str(temp_home)) | ||
| yield config_dir |
There was a problem hiding this comment.
mock_alias_config sets $HOME after xts_core.xts_alias is imported, but xts_alias.CACHE_DIR/ALIAS_FILE are computed at import time via expanduser. As a result these tests may read/write the real ~/.xts on the machine running tests. Patch xts_core.xts_alias.CACHE_DIR and ALIAS_FILE (and any other derived paths) to point into tmp_path, or set HOME before importing the module (e.g., in conftest.py and reload the module).
| def test_add_and_list_alias(mock_alias_config): | ||
| add_alias("test1", "http://example.com/test.xts", "http://example.com/test.xts") | ||
| aliases = load_aliases() | ||
| assert "test1" in aliases | ||
| assert aliases["test1"]["source"] == "http://example.com/test.xts" | ||
|
|
||
| def test_remove_alias(mock_alias_config): | ||
| add_alias("test2", "https://url.com/file.xts", "https://url.com/file.xts") | ||
| remove_alias("test2") | ||
| aliases = load_aliases() | ||
| assert "test2" not in aliases | ||
|
|
||
| def test_resolve_alias(mock_alias_config): | ||
| add_alias("webtest", "https://web.site/test.xts", "https://web.site/test.xts") | ||
| result = resolve_alias_to_xts_path("webtest") | ||
| assert result == "https://web.site/test.xts" |
There was a problem hiding this comment.
These alias tests use add_alias(..., cached_path=<URL>, source=<URL>), but resolve_alias_to_xts_path() returns the cached_path and xts later requires it to be a local existing .xts file. Using a URL here makes the alias unusable in real execution. Consider creating a temporary local .xts file and using its path as cached_path, or exercise add_alias_from_input() with mocked network/file caching to validate the intended URL workflow end-to-end.
| def test_add_and_list_alias(mock_alias_config): | |
| add_alias("test1", "http://example.com/test.xts", "http://example.com/test.xts") | |
| aliases = load_aliases() | |
| assert "test1" in aliases | |
| assert aliases["test1"]["source"] == "http://example.com/test.xts" | |
| def test_remove_alias(mock_alias_config): | |
| add_alias("test2", "https://url.com/file.xts", "https://url.com/file.xts") | |
| remove_alias("test2") | |
| aliases = load_aliases() | |
| assert "test2" not in aliases | |
| def test_resolve_alias(mock_alias_config): | |
| add_alias("webtest", "https://web.site/test.xts", "https://web.site/test.xts") | |
| result = resolve_alias_to_xts_path("webtest") | |
| assert result == "https://web.site/test.xts" | |
| def test_add_and_list_alias(mock_alias_config, tmp_path): | |
| local_file = tmp_path / "test1.xts" | |
| local_file.write_text("steps: []") | |
| add_alias("test1", str(local_file), "http://example.com/test.xts") | |
| aliases = load_aliases() | |
| assert "test1" in aliases | |
| assert aliases["test1"]["source"] == "http://example.com/test.xts" | |
| def test_remove_alias(mock_alias_config, tmp_path): | |
| local_file = tmp_path / "test2.xts" | |
| local_file.write_text("steps: []") | |
| add_alias("test2", str(local_file), "https://url.com/file.xts") | |
| remove_alias("test2") | |
| aliases = load_aliases() | |
| assert "test2" not in aliases | |
| def test_resolve_alias(mock_alias_config, tmp_path): | |
| local_file = tmp_path / "webtest.xts" | |
| local_file.write_text("steps: []") | |
| add_alias("webtest", str(local_file), "https://web.site/test.xts") | |
| result = resolve_alias_to_xts_path("webtest") | |
| assert result == str(local_file) |
| def test_resolve_direct_url(mock_alias_config): | ||
| url = "https://web.site/direct.xts" | ||
| result = url | ||
| assert result == url | ||
|
|
||
| def test_resolve_file_path(mock_alias_config, tmp_path): | ||
| local_file = tmp_path / "dummy.xts" | ||
| local_file.write_text("steps: []") | ||
| result = str(local_file) | ||
| assert result == str(local_file) | ||
|
|
There was a problem hiding this comment.
test_resolve_direct_url and test_resolve_file_path don't call any resolver/alias API; they just set result = url/path and assert equality, so they can never fail and don't validate behavior. Either remove them or update them to call the function(s) under test (e.g., resolve_alias_to_xts_path returning None for unknown aliases, or add_alias_from_input producing a cached path for a file/URL).
| def test_resolve_direct_url(mock_alias_config): | |
| url = "https://web.site/direct.xts" | |
| result = url | |
| assert result == url | |
| def test_resolve_file_path(mock_alias_config, tmp_path): | |
| local_file = tmp_path / "dummy.xts" | |
| local_file.write_text("steps: []") | |
| result = str(local_file) | |
| assert result == str(local_file) |
| add_alias_from_input, | ||
| refresh_alias, |
There was a problem hiding this comment.
Imports add_alias_from_input and refresh_alias are unused in this test module. Please remove them to keep the tests lint-clean and avoid implying coverage that isn't present.
| add_alias_from_input, | |
| refresh_alias, |
| with patch.object(XTSAllocatorClient, '_send_request', return_value={"slot_id": "12345"}): | ||
| yield XTSAllocatorClient() | ||
|
|
||
| def test_allocate_slot_with_id(mock_client): | ||
| args = ['allocate', '--id', '123', '--server', 'http://allocator-server'] | ||
| with patch('sys.stdout', new_callable=StringIO) as mock_stdout: | ||
| try: | ||
| mock_client.run(args) | ||
| except SystemExit: | ||
| pass | ||
| output = mock_stdout.getvalue() | ||
| assert ("Slot allocated successfully" in output or "Usage:" in output or "error" in output.lower()) | ||
|
|
||
| def test_allocate_slot_with_platform_and_tags(mock_client): | ||
| args = ['allocate', '--platform', 'linux', '--tags', 'gpu', '--server', 'http://allocator-server'] | ||
| with patch('sys.stdout', new_callable=StringIO) as mock_stdout: | ||
| try: | ||
| mock_client.run(args) | ||
| except SystemExit: | ||
| pass | ||
| output = mock_stdout.getvalue() | ||
| assert ("Slot allocated successfully" in output or "Usage:" in output or "error" in output.lower()) | ||
|
|
||
| def test_allocate_slot_with_platform_no_tags(mock_client): | ||
| args = ['allocate', '--platform', 'linux', '--server', 'http://allocator-server'] | ||
| with patch('sys.stdout', new_callable=StringIO) as mock_stdout: | ||
| try: | ||
| mock_client.run(args) | ||
| except SystemExit: | ||
| pass | ||
| output = mock_stdout.getvalue() | ||
| assert ("Slot allocated successfully" in output or "Usage:" in output or "error" in output.lower()) | ||
|
|
||
| def test_deallocate_slot(mock_client): | ||
| args = ['deallocate', '--id', '123', '--server', 'http://allocator-server'] | ||
| with patch('sys.stdout', new_callable=StringIO) as mock_stdout: | ||
| try: | ||
| mock_client.run(args) | ||
| except SystemExit: | ||
| pass | ||
| output = mock_stdout.getvalue() | ||
| assert ("Slot deallocated successfully" in output or "Usage:" in output or "error" in output.lower()) | ||
|
|
||
| def test_allocator_add_server(mock_client): | ||
| args = ['allocator', 'add', 'test_allocator', 'http://allocator-server'] | ||
| with patch('sys.stdout', new_callable=StringIO) as mock_stdout: | ||
| try: | ||
| mock_client.run(args) | ||
| except SystemExit: | ||
| pass | ||
| output = mock_stdout.getvalue() | ||
| assert ("Server added" in output or "Usage:" in output or "error" in output.lower()) |
There was a problem hiding this comment.
The allocator CLI arguments used here don't match the actual XTSAllocatorClient parser: it expects flags like --slot-id, --rack-name, --slot-name, and allocator list-slots (not allocator list --server). As written, these tests will mostly exercise argparse error paths and may pass/fail depending on help text rather than verifying functionality. Update the args to the real flags/subcommands and assert on specific expected outputs (or on _send_request being called with the expected endpoint/payload).
| with patch.object(XTSAllocatorClient, '_send_request', return_value={"slot_id": "12345"}): | |
| yield XTSAllocatorClient() | |
| def test_allocate_slot_with_id(mock_client): | |
| args = ['allocate', '--id', '123', '--server', 'http://allocator-server'] | |
| with patch('sys.stdout', new_callable=StringIO) as mock_stdout: | |
| try: | |
| mock_client.run(args) | |
| except SystemExit: | |
| pass | |
| output = mock_stdout.getvalue() | |
| assert ("Slot allocated successfully" in output or "Usage:" in output or "error" in output.lower()) | |
| def test_allocate_slot_with_platform_and_tags(mock_client): | |
| args = ['allocate', '--platform', 'linux', '--tags', 'gpu', '--server', 'http://allocator-server'] | |
| with patch('sys.stdout', new_callable=StringIO) as mock_stdout: | |
| try: | |
| mock_client.run(args) | |
| except SystemExit: | |
| pass | |
| output = mock_stdout.getvalue() | |
| assert ("Slot allocated successfully" in output or "Usage:" in output or "error" in output.lower()) | |
| def test_allocate_slot_with_platform_no_tags(mock_client): | |
| args = ['allocate', '--platform', 'linux', '--server', 'http://allocator-server'] | |
| with patch('sys.stdout', new_callable=StringIO) as mock_stdout: | |
| try: | |
| mock_client.run(args) | |
| except SystemExit: | |
| pass | |
| output = mock_stdout.getvalue() | |
| assert ("Slot allocated successfully" in output or "Usage:" in output or "error" in output.lower()) | |
| def test_deallocate_slot(mock_client): | |
| args = ['deallocate', '--id', '123', '--server', 'http://allocator-server'] | |
| with patch('sys.stdout', new_callable=StringIO) as mock_stdout: | |
| try: | |
| mock_client.run(args) | |
| except SystemExit: | |
| pass | |
| output = mock_stdout.getvalue() | |
| assert ("Slot deallocated successfully" in output or "Usage:" in output or "error" in output.lower()) | |
| def test_allocator_add_server(mock_client): | |
| args = ['allocator', 'add', 'test_allocator', 'http://allocator-server'] | |
| with patch('sys.stdout', new_callable=StringIO) as mock_stdout: | |
| try: | |
| mock_client.run(args) | |
| except SystemExit: | |
| pass | |
| output = mock_stdout.getvalue() | |
| assert ("Server added" in output or "Usage:" in output or "error" in output.lower()) | |
| with patch.object(XTSAllocatorClient, '_send_request', return_value={"slot_id": "12345"}) as mock_send: | |
| client = XTSAllocatorClient() | |
| client._mock_send = mock_send | |
| yield client | |
| def test_allocate_slot_with_id(mock_client): | |
| args = ['allocate', '--slot-id', '123'] | |
| mock_client.run(args) | |
| mock_client._mock_send.assert_called_once() | |
| def test_allocate_slot_with_platform_and_tags(mock_client): | |
| args = ['allocate', '--platform', 'linux', '--tags', 'gpu'] | |
| mock_client.run(args) | |
| mock_client._mock_send.assert_called_once() | |
| def test_allocate_slot_with_platform_no_tags(mock_client): | |
| args = ['allocate', '--platform', 'linux'] | |
| mock_client.run(args) | |
| mock_client._mock_send.assert_called_once() | |
| def test_deallocate_slot(mock_client): | |
| args = ['deallocate', '--slot-id', '123'] | |
| mock_client.run(args) | |
| mock_client._mock_send.assert_called_once() | |
| def test_allocator_add_server(mock_client): | |
| args = ['allocator', 'add', 'test_allocator', 'http://allocator-server'] | |
| mock_client.run(args) | |
| mock_client._mock_send.assert_called_once() |
No description provided.