diff --git a/.agents/skills/pr-feedback-triage/SKILL.md b/.agents/skills/pr-feedback-triage/SKILL.md index a64e85c..bf32136 100644 --- a/.agents/skills/pr-feedback-triage/SKILL.md +++ b/.agents/skills/pr-feedback-triage/SKILL.md @@ -1,6 +1,7 @@ --- name: pr-feedback-triage description: Triage pull request review comments into fixes, replies, clarification requests, or open follow-ups while respecting safe execution modes. +allowed-tools: Bash(git:*), Bash(gh:*), mcp__github__*, Read, Grep, Glob, Edit, MultiEdit, Write --- # PR Feedback Triage @@ -44,7 +45,7 @@ When a mode disables an action, skip that destructive or externally visible acti Gather the complete feedback set before editing: - Fetch unresolved review threads, requested-change reviews, inline comments, copied comments, and PR-level summary comments. -- Use platform-native APIs/CLI when available. Paginate results; do not inspect only the first page of threads or comments. +- Use whichever authenticated GitHub-capable interface is available and reliable. This skill explicitly permits both `gh` and GitHub MCP tools; paginate results and do not inspect only the first page of threads or comments. - For bot reviewers that post both summary comments and inline comments, prefer inline comments for actionable triage. Incorporate summary findings only when they contain distinct severity, rationale, or fix instructions not already captured from inline comments. - Summary comments may be excluded from triage when they do not add distinct actionable context. - Preserve every thread/comment identifier needed to reply or resolve later. @@ -99,7 +100,7 @@ For duplicate findings, execute the terminal action for every source thread ID, ## GitHub Action Guidance -Prefer platform-native APIs or `gh` commands that expose review-thread resolution state. For GitHub inline review threads, use the thread node ID and the GraphQL `resolveReviewThread` mutation rather than assuming that a reply resolves the conversation. +Use whichever authenticated GitHub-capable interface is available and reliable, preferably `gh` or GitHub MCP. Prefer interfaces that expose review-thread resolution state. For GitHub inline review threads, use the thread node ID and the GraphQL `resolveReviewThread` mutation, or an equivalent GitHub MCP resolve-thread tool, rather than assuming that a reply resolves the conversation. A reliable pattern is: diff --git a/tests/test_cli.py b/tests/test_cli.py index 3bdef2c..734911e 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -112,207 +112,176 @@ class TestCommands: group="*USD*", ) - def test_symbol_info( + @pytest.mark.parametrize( + ("command", "method"), + [ + ("symbol-info", "symbol_info_as_df"), + ("symbol-info-tick", "symbol_info_tick_as_df"), + ("market-book", "market_book_get_as_df"), + ], + ) + def test_symbol_command( self, tmp_path: Path, mock_client: MagicMock, + command: str, + method: str, ) -> None: - """Test symbol-info command.""" + """Test --symbol commands invoke the expected client method with symbol.""" output = tmp_path / "out.csv" result = runner.invoke( app, - ["-o", str(output), "symbol-info", "--symbol", "EURUSD"], + ["-o", str(output), command, "--symbol", "EURUSD"], ) assert result.exit_code == 0, result.output - mock_client.symbol_info_as_df.assert_called_once_with( - symbol="EURUSD", - ) + getattr(mock_client, method).assert_called_once_with(symbol="EURUSD") - def test_rates_from( + @pytest.mark.parametrize( + ("args", "method", "expected_kwargs"), + [ + ( + [ + "rates-from", + "--symbol", + "EURUSD", + "--timeframe", + "M1", + "--date-from", + "2024-01-01", + "--count", + "100", + ], + "copy_rates_from_as_df", + { + "symbol": "EURUSD", + "timeframe": 1, + "date_from": datetime(2024, 1, 1, tzinfo=UTC), + "count": 100, + }, + ), + ( + [ + "rates-from-pos", + "--symbol", + "GBPUSD", + "--timeframe", + "H1", + "--start-pos", + "0", + "--count", + "50", + ], + "copy_rates_from_pos_as_df", + { + "symbol": "GBPUSD", + "timeframe": 16385, + "start_pos": 0, + "count": 50, + }, + ), + ( + [ + "latest-rates", + "--symbol", + "GBPUSD", + "--timeframe", + "H1", + "--count", + "50", + "--start-pos", + "2", + ], + "copy_rates_from_pos_as_df", + { + "symbol": "GBPUSD", + "timeframe": 16385, + "start_pos": 2, + "count": 50, + }, + ), + ( + [ + "rates-range", + "--symbol", + "USDJPY", + "--timeframe", + "D1", + "--date-from", + "2024-01-01", + "--date-to", + "2024-02-01", + ], + "copy_rates_range_as_df", + { + "symbol": "USDJPY", + "timeframe": 16408, + "date_from": datetime(2024, 1, 1, tzinfo=UTC), + "date_to": datetime(2024, 2, 1, tzinfo=UTC), + }, + ), + ( + [ + "ticks-from", + "--symbol", + "EURUSD", + "--date-from", + "2024-01-01", + "--count", + "100", + "--flags", + "ALL", + ], + "copy_ticks_from_as_df", + { + "symbol": "EURUSD", + "date_from": datetime(2024, 1, 1, tzinfo=UTC), + "count": 100, + "flags": -1, + }, + ), + ( + [ + "ticks-range", + "--symbol", + "EURUSD", + "--date-from", + "2024-01-01", + "--date-to", + "2024-02-01", + "--flags", + "INFO", + ], + "copy_ticks_range_as_df", + { + "symbol": "EURUSD", + "date_from": datetime(2024, 1, 1, tzinfo=UTC), + "date_to": datetime(2024, 2, 1, tzinfo=UTC), + "flags": 1, + }, + ), + ], + ids=[ + "rates-from", + "rates-from-pos", + "latest-rates", + "rates-range", + "ticks-from", + "ticks-range", + ], + ) + def test_rates_ticks_delegate( self, tmp_path: Path, mock_client: MagicMock, + args: list[str], + method: str, + expected_kwargs: dict[str, object], ) -> None: - """Test rates-from command.""" + """Rate/tick delegate commands invoke the expected client method with kwargs.""" output = tmp_path / "out.csv" - result = runner.invoke( - app, - [ - "-o", - str(output), - "rates-from", - "--symbol", - "EURUSD", - "--timeframe", - "M1", - "--date-from", - "2024-01-01", - "--count", - "100", - ], - ) + result = runner.invoke(app, ["-o", str(output), *args]) assert result.exit_code == 0, result.output - mock_client.copy_rates_from_as_df.assert_called_once_with( - symbol="EURUSD", - timeframe=1, - date_from=datetime(2024, 1, 1, tzinfo=UTC), - count=100, - ) - - def test_rates_from_pos( - self, - tmp_path: Path, - mock_client: MagicMock, - ) -> None: - """Test rates-from-pos command.""" - output = tmp_path / "out.csv" - result = runner.invoke( - app, - [ - "-o", - str(output), - "rates-from-pos", - "--symbol", - "GBPUSD", - "--timeframe", - "H1", - "--start-pos", - "0", - "--count", - "50", - ], - ) - assert result.exit_code == 0, result.output - mock_client.copy_rates_from_pos_as_df.assert_called_once_with( - symbol="GBPUSD", - timeframe=16385, - start_pos=0, - count=50, - ) - - def test_latest_rates( - self, - tmp_path: Path, - mock_client: MagicMock, - ) -> None: - """Test latest-rates command.""" - output = tmp_path / "out.csv" - result = runner.invoke( - app, - [ - "-o", - str(output), - "latest-rates", - "--symbol", - "GBPUSD", - "--timeframe", - "H1", - "--count", - "50", - "--start-pos", - "2", - ], - ) - assert result.exit_code == 0, result.output - mock_client.copy_rates_from_pos_as_df.assert_called_once_with( - symbol="GBPUSD", - timeframe=16385, - start_pos=2, - count=50, - ) - - def test_rates_range( - self, - tmp_path: Path, - mock_client: MagicMock, - ) -> None: - """Test rates-range command.""" - output = tmp_path / "out.csv" - result = runner.invoke( - app, - [ - "-o", - str(output), - "rates-range", - "--symbol", - "USDJPY", - "--timeframe", - "D1", - "--date-from", - "2024-01-01", - "--date-to", - "2024-02-01", - ], - ) - assert result.exit_code == 0, result.output - mock_client.copy_rates_range_as_df.assert_called_once_with( - symbol="USDJPY", - timeframe=16408, - date_from=datetime(2024, 1, 1, tzinfo=UTC), - date_to=datetime(2024, 2, 1, tzinfo=UTC), - ) - - def test_ticks_from( - self, - tmp_path: Path, - mock_client: MagicMock, - ) -> None: - """Test ticks-from command.""" - output = tmp_path / "out.csv" - result = runner.invoke( - app, - [ - "-o", - str(output), - "ticks-from", - "--symbol", - "EURUSD", - "--date-from", - "2024-01-01", - "--count", - "100", - "--flags", - "ALL", - ], - ) - assert result.exit_code == 0, result.output - mock_client.copy_ticks_from_as_df.assert_called_once_with( - symbol="EURUSD", - date_from=datetime(2024, 1, 1, tzinfo=UTC), - count=100, - flags=-1, - ) - - def test_ticks_range( - self, - tmp_path: Path, - mock_client: MagicMock, - ) -> None: - """Test ticks-range command.""" - output = tmp_path / "out.csv" - result = runner.invoke( - app, - [ - "-o", - str(output), - "ticks-range", - "--symbol", - "EURUSD", - "--date-from", - "2024-01-01", - "--date-to", - "2024-02-01", - "--flags", - "INFO", - ], - ) - assert result.exit_code == 0, result.output - mock_client.copy_ticks_range_as_df.assert_called_once_with( - symbol="EURUSD", - date_from=datetime(2024, 1, 1, tzinfo=UTC), - date_to=datetime(2024, 2, 1, tzinfo=UTC), - flags=1, - ) + getattr(mock_client, method).assert_called_once_with(**expected_kwargs) def test_ticks_recent( self, @@ -373,67 +342,36 @@ class TestCommands: mock_client.order_calc_margin.assert_any_call(0, "EURUSD", 0.01, 1.1010) mock_client.order_calc_margin.assert_any_call(1, "EURUSD", 0.01, 1.1000) - def test_orders( + @pytest.mark.parametrize( + ("args", "method"), + [ + (["orders", "--symbol", "EURUSD"], "orders_get_as_df"), + ( + [ + "history-orders", + "--date-from", + "2024-01-01", + "--date-to", + "2024-02-01", + ], + "history_orders_get_as_df", + ), + (["history-deals", "--ticket", "12345"], "history_deals_get_as_df"), + ], + ids=["orders", "history-orders", "history-deals"], + ) + def test_history_delegate( self, tmp_path: Path, mock_client: MagicMock, + args: list[str], + method: str, ) -> None: - """Test orders command.""" + """orders/history-orders/history-deals delegate to the client method.""" output = tmp_path / "out.csv" - result = runner.invoke( - app, - [ - "-o", - str(output), - "orders", - "--symbol", - "EURUSD", - ], - ) + result = runner.invoke(app, ["-o", str(output), *args]) assert result.exit_code == 0, result.output - mock_client.orders_get_as_df.assert_called_once() - - def test_history_orders( - self, - tmp_path: Path, - mock_client: MagicMock, - ) -> None: - """Test history-orders command.""" - output = tmp_path / "out.csv" - result = runner.invoke( - app, - [ - "-o", - str(output), - "history-orders", - "--date-from", - "2024-01-01", - "--date-to", - "2024-02-01", - ], - ) - assert result.exit_code == 0, result.output - mock_client.history_orders_get_as_df.assert_called_once() - - def test_history_deals( - self, - tmp_path: Path, - mock_client: MagicMock, - ) -> None: - """Test history-deals command.""" - output = tmp_path / "out.csv" - result = runner.invoke( - app, - [ - "-o", - str(output), - "history-deals", - "--ticket", - "12345", - ], - ) - assert result.exit_code == 0, result.output - mock_client.history_deals_get_as_df.assert_called_once() + getattr(mock_client, method).assert_called_once() def test_recent_history_deals( self, @@ -513,89 +451,79 @@ class TestCommands: "symbols_total": 42, } - def test_symbol_info_tick( + @pytest.mark.parametrize( + ("command", "method", "payload", "use_file"), + [ + ( + "order-check", + "order_check_as_df", + {"action": 1, "symbol": "EURUSD", "volume": 0.1}, + False, + ), + ( + "order-send", + "order_send_as_df", + {"action": 1, "symbol": "EURUSD", "volume": 0.1}, + False, + ), + ( + "order-check", + "order_check_as_df", + {"action": 2, "symbol": "EURUSD"}, + True, + ), + ("order-send", "order_send_as_df", {"action": 2, "symbol": "EURUSD"}, True), + ], + ids=["check-inline", "send-inline", "check-file", "send-file"], + ) + def test_order_request( self, tmp_path: Path, mock_client: MagicMock, + command: str, + method: str, + payload: dict[str, object], + use_file: bool, ) -> None: - """Test symbol-info-tick command.""" + """Test order-check/order-send accept inline and file-based JSON requests.""" output = tmp_path / "out.csv" - result = runner.invoke( - app, - ["-o", str(output), "symbol-info-tick", "--symbol", "EURUSD"], - ) + args = ["-o", str(output), command] + if use_file: + req_path = tmp_path / "req.json" + req_path.write_text(json.dumps(payload), encoding="utf-8") + args += ["--request", f"@{req_path}"] + else: + args += ["--request", json.dumps(payload)] + if command == "order-send": + args.append("--yes") + result = runner.invoke(app, args) assert result.exit_code == 0, result.output - mock_client.symbol_info_tick_as_df.assert_called_once_with( - symbol="EURUSD", - ) + getattr(mock_client, method).assert_called_once_with(request=payload) - def test_market_book( - self, - tmp_path: Path, - mock_client: MagicMock, - ) -> None: - """Test market-book command.""" - output = tmp_path / "out.csv" - result = runner.invoke( - app, - ["-o", str(output), "market-book", "--symbol", "EURUSD"], - ) - assert result.exit_code == 0, result.output - mock_client.market_book_get_as_df.assert_called_once_with( - symbol="EURUSD", - ) - - def test_order_check( - self, - tmp_path: Path, - mock_client: MagicMock, - ) -> None: - """Test order-check command with inline JSON.""" - output = tmp_path / "out.csv" - request = json.dumps({"action": 1, "symbol": "EURUSD", "volume": 0.1}) - result = runner.invoke( - app, - ["-o", str(output), "order-check", "--request", request], - ) - assert result.exit_code == 0, result.output - mock_client.order_check_as_df.assert_called_once_with( - request={"action": 1, "symbol": "EURUSD", "volume": 0.1}, - ) - - def test_order_check_file_reference( - self, - tmp_path: Path, - mock_client: MagicMock, - ) -> None: - """Test order-check command with file-based JSON.""" - output = tmp_path / "out.csv" - req_path = tmp_path / "req.json" - req_path.write_text( - json.dumps({"action": 2, "symbol": "EURUSD"}), - encoding="utf-8", - ) - result = runner.invoke( - app, - ["-o", str(output), "order-check", "--request", f"@{req_path}"], - ) - assert result.exit_code == 0, result.output - mock_client.order_check_as_df.assert_called_once_with( - request={"action": 2, "symbol": "EURUSD"}, - ) - - def test_order_check_invalid_request( + @pytest.mark.parametrize( + ("command", "raw_request", "match"), + [ + ("order-check", "not-json", "Invalid JSON request"), + ("order-send", "[1,2]", "must be a JSON object"), + ], + ids=["check-invalid-json", "send-non-object"], + ) + def test_order_invalid_request( self, tmp_path: Path, mock_client: MagicMock, # noqa: ARG002 + command: str, + raw_request: str, + match: str, ) -> None: - """Test order-check rejects invalid JSON.""" + """Test order-check/order-send reject malformed JSON requests.""" output = tmp_path / "out.csv" - result = runner.invoke( - app, - ["-o", str(output), "order-check", "--request", "not-json"], - ) + args = ["-o", str(output), command, "--request", raw_request] + if command == "order-send": + args.append("--yes") + result = runner.invoke(app, args) assert result.exit_code != 0 - assert "Invalid JSON request" in normalize_cli_output(result.output) + assert match in normalize_cli_output(result.output) def test_order_check_missing_request_file( self, @@ -614,58 +542,6 @@ class TestCommands: result.output, ) - def test_order_send( - self, - tmp_path: Path, - mock_client: MagicMock, - ) -> None: - """Test order-send command with file-based JSON.""" - output = tmp_path / "out.csv" - req_path = tmp_path / "req.json" - req_path.write_text( - json.dumps({"action": 2, "symbol": "EURUSD"}), - encoding="utf-8", - ) - result = runner.invoke( - app, - [ - "-o", - str(output), - "order-send", - "--request", - f"@{req_path}", - "--yes", - ], - ) - assert result.exit_code == 0, result.output - mock_client.order_send_as_df.assert_called_once_with( - request={"action": 2, "symbol": "EURUSD"}, - ) - - def test_order_send_inline_json( - self, - tmp_path: Path, - mock_client: MagicMock, - ) -> None: - """Test order-send command with inline JSON.""" - output = tmp_path / "out.csv" - request = json.dumps({"action": 1, "symbol": "EURUSD", "volume": 0.1}) - result = runner.invoke( - app, - [ - "-o", - str(output), - "order-send", - "--request", - request, - "--yes", - ], - ) - assert result.exit_code == 0, result.output - mock_client.order_send_as_df.assert_called_once_with( - request={"action": 1, "symbol": "EURUSD", "volume": 0.1}, - ) - def test_order_send_requires_yes( self, tmp_path: Path, @@ -684,20 +560,6 @@ class TestCommands: ) mock_client.order_send_as_df.assert_not_called() - def test_order_send_invalid_request( - self, - tmp_path: Path, - mock_client: MagicMock, # noqa: ARG002 - ) -> None: - """Test order-send rejects invalid JSON.""" - output = tmp_path / "out.csv" - result = runner.invoke( - app, - ["-o", str(output), "order-send", "--request", "[1,2]", "--yes"], - ) - assert result.exit_code != 0 - assert "must be a JSON object" in normalize_cli_output(result.output) - # --------------------------------------------------------------------------- # Help text / scope tests @@ -776,153 +638,95 @@ class TestClosePositions: mocker.patch("mt5cli.cli.create_trading_client", return_value=client) return client - def test_dry_run_does_not_require_yes( + @pytest.mark.parametrize( + ("extra_args", "order_send_called"), + [ + pytest.param(["--dry-run"], False, id="dry-run without --yes"), + pytest.param(["--dry-run", "--yes"], False, id="dry-run with --yes"), + ], + ) + def test_dry_run_does_not_send_orders( self, tmp_path: Path, trading_client: MagicMock, + extra_args: list[str], + order_send_called: bool, ) -> None: - """Test --dry-run mode succeeds without --yes.""" + """Test --dry-run mode succeeds and never sends orders.""" output = tmp_path / "close.json" result = runner.invoke( app, - ["-o", str(output), "close-positions", "--symbol", "JP225", "--dry-run"], + [ + "-o", + str(output), + "close-positions", + "--symbol", + "JP225", + *extra_args, + ], ) assert result.exit_code == 0, result.output assert output.exists() - trading_client.order_send.assert_not_called() + assert trading_client.order_send.called == order_send_called trading_client.shutdown.assert_called_once() - def test_live_requires_yes( + @pytest.mark.parametrize( + ("extra_args", "yes_included"), + [ + pytest.param([], False, id="live-requires-yes"), + pytest.param(["--yes"], True, id="live-with-yes-calls-order-send"), + ], + ) + def test_close_positions_yes_gate( self, tmp_path: Path, trading_client: MagicMock, + extra_args: list[str], + yes_included: bool, ) -> None: - """Test live close-positions fails without --yes.""" - output = tmp_path / "close.json" - result = runner.invoke( - app, - ["-o", str(output), "close-positions", "--symbol", "JP225"], - ) - assert result.exit_code != 0 - assert "Pass --yes" in normalize_cli_output(result.output) - trading_client.order_send.assert_not_called() - - def test_live_with_yes_calls_order_send( - self, - tmp_path: Path, - trading_client: MagicMock, - ) -> None: - """Test --yes triggers live execution for matching positions.""" + """Test --yes gates live close-positions execution.""" trading_client.order_send.return_value = {"retcode": 10009, "comment": "ok"} output = tmp_path / "close.json" result = runner.invoke( app, - ["-o", str(output), "close-positions", "--symbol", "JP225", "--yes"], + ["-o", str(output), "close-positions", "--symbol", "JP225", *extra_args], ) - assert result.exit_code == 0, result.output - trading_client.order_send.assert_called_once() - trading_client.shutdown.assert_called_once() + if yes_included: + assert result.exit_code == 0, result.output + trading_client.order_send.assert_called_once() + trading_client.shutdown.assert_called_once() + else: + assert result.exit_code != 0 + assert "Pass --yes" in normalize_cli_output(result.output) + trading_client.order_send.assert_not_called() - def test_symbol_filter_passed_through( + @pytest.mark.parametrize( + ("extra_args", "expected_symbols"), + [ + (["--symbol", "JP225"], {"JP225"}), + (["--symbol", "JP225", "--symbol", "EURUSD"], {"JP225", "EURUSD"}), + (["--ticket", "2"], {"EURUSD"}), + (["--symbol", "JP225", "--ticket", "1"], {"JP225"}), + ], + ids=["single-symbol", "multiple-symbols", "ticket", "symbol-and-ticket"], + ) + def test_close_positions_filter( self, tmp_path: Path, trading_client: MagicMock, + extra_args: list[str], + expected_symbols: set[str], ) -> None: - """Test --symbol values are used to filter positions.""" + """--symbol/--ticket filters select the expected positions under --dry-run.""" output = tmp_path / "close.json" result = runner.invoke( app, - [ - "-o", - str(output), - "close-positions", - "--symbol", - "JP225", - "--dry-run", - ], + ["-o", str(output), "close-positions", "--dry-run", *extra_args], ) assert result.exit_code == 0, result.output data = json.loads(output.read_text()) - assert len(data) == 1 - assert data[0]["symbol"] == "JP225" - trading_client.shutdown.assert_called_once() - - def test_multiple_symbols_filter( - self, - tmp_path: Path, - trading_client: MagicMock, - ) -> None: - """Test multiple --symbol options are combined.""" - output = tmp_path / "close.json" - result = runner.invoke( - app, - [ - "-o", - str(output), - "close-positions", - "--symbol", - "JP225", - "--symbol", - "EURUSD", - "--dry-run", - ], - ) - assert result.exit_code == 0, result.output - data = json.loads(output.read_text()) - assert len(data) == 2 - symbols = {row["symbol"] for row in data} - assert symbols == {"JP225", "EURUSD"} - trading_client.shutdown.assert_called_once() - - def test_ticket_filter_passed_through( - self, - tmp_path: Path, - trading_client: MagicMock, - ) -> None: - """Test --ticket values are used to filter positions.""" - output = tmp_path / "close.json" - result = runner.invoke( - app, - [ - "-o", - str(output), - "close-positions", - "--ticket", - "2", - "--dry-run", - ], - ) - assert result.exit_code == 0, result.output - data = json.loads(output.read_text()) - assert len(data) == 1 - assert data[0]["symbol"] == "EURUSD" - trading_client.shutdown.assert_called_once() - - def test_symbol_and_ticket_combined( - self, - tmp_path: Path, - trading_client: MagicMock, - ) -> None: - """Test --symbol and --ticket apply AND semantics when combined.""" - output = tmp_path / "close.json" - result = runner.invoke( - app, - [ - "-o", - str(output), - "close-positions", - "--symbol", - "JP225", - "--ticket", - "1", - "--dry-run", - ], - ) - assert result.exit_code == 0, result.output - data = json.loads(output.read_text()) - # symbol=JP225 AND ticket=1 → exactly one match - assert len(data) == 1 - assert data[0]["symbol"] == "JP225" + assert {row["symbol"] for row in data} == expected_symbols + assert len(data) == len(expected_symbols) trading_client.shutdown.assert_called_once() def test_missing_symbol_and_ticket_fails( @@ -990,29 +794,6 @@ class TestClosePositions: assert result.exit_code != 0 client.shutdown.assert_called_once() - def test_dry_run_wins_over_yes( - self, - tmp_path: Path, - trading_client: MagicMock, - ) -> None: - """Test that --dry-run takes precedence when combined with --yes.""" - output = tmp_path / "close.json" - result = runner.invoke( - app, - [ - "-o", - str(output), - "close-positions", - "--symbol", - "JP225", - "--dry-run", - "--yes", - ], - ) - assert result.exit_code == 0, result.output - trading_client.order_send.assert_not_called() - trading_client.shutdown.assert_called_once() - def test_no_matching_positions_exports_empty_result( self, tmp_path: Path, @@ -1238,72 +1019,76 @@ class TestCollectHistory: """Create a mocked Mt5DataClient with history-style DataFrames.""" return _build_history_client(mocker) - def test_collect_history_writes_default_tables( - self, - tmp_path: Path, - history_client: MagicMock, - ) -> None: - """Test that collect-history default excludes ticks.""" - output = tmp_path / "history.db" - result = runner.invoke( - app, - [ - "-o", - str(output), - "collect-history", - "--symbol", - "EURUSD", - "--symbol", - "GBPUSD", - "--date-from", - "2024-01-01", - "--date-to", - "2024-02-01", - ], - ) - assert result.exit_code == 0, result.output - assert history_client.copy_rates_range_as_df.call_count == 2 - assert history_client.copy_ticks_range_as_df.call_count == 0 - with sqlite3.connect(output) as conn: - tables = { - row[0] - for row in conn.execute( - "SELECT name FROM sqlite_master WHERE type='table'", - ).fetchall() - } - assert {"rates", "history_orders", "history_deals"} <= tables - assert "ticks" not in tables - - def test_collect_history_explicit_ticks_dataset( - self, - tmp_path: Path, - history_client: MagicMock, - ) -> None: - """Test that --dataset ticks writes the ticks table with the correct flags.""" - output = tmp_path / "history.db" - result = runner.invoke( - app, - [ - "-o", - str(output), - "collect-history", - "--symbol", - "EURUSD", - "--date-from", - "2024-01-01", - "--date-to", - "2024-02-01", - "--dataset", + @pytest.mark.parametrize( + ( + "extra_args", + "symbols", + "expected_rates_calls", + "expected_ticks_calls", + "required_tables", + "forbidden_table", + "verify_ticks_call", + ), + [ + pytest.param( + [], + ["EURUSD", "GBPUSD"], + 2, + 0, + {"rates", "history_orders", "history_deals"}, "ticks", - ], - ) + False, + id="default-excludes-ticks", + ), + pytest.param( + ["--dataset", "ticks"], + ["EURUSD"], + 0, + 1, + {"ticks"}, + "rates", + True, + id="explicit-ticks", + ), + ], + ) + def test_collect_history_default_and_ticks_dataset( + self, + tmp_path: Path, + history_client: MagicMock, + extra_args: list[str], + symbols: list[str], + expected_rates_calls: int, + expected_ticks_calls: int, + required_tables: set[str], + forbidden_table: str, + verify_ticks_call: bool, + ) -> None: + """Test default vs --dataset ticks selection for collect-history.""" + output = tmp_path / "history.db" + args = [ + "-o", + str(output), + "collect-history", + "--date-from", + "2024-01-01", + "--date-to", + "2024-02-01", + ] + for symbol in symbols: + args.extend(["--symbol", symbol]) + args.extend(extra_args) + result = runner.invoke(app, args) assert result.exit_code == 0, result.output - history_client.copy_ticks_range_as_df.assert_called_once_with( - symbol="EURUSD", - date_from=datetime(2024, 1, 1, tzinfo=UTC), - date_to=datetime(2024, 2, 1, tzinfo=UTC), - flags=-1, - ) + assert history_client.copy_rates_range_as_df.call_count == expected_rates_calls + assert history_client.copy_ticks_range_as_df.call_count == expected_ticks_calls + if verify_ticks_call: + history_client.copy_ticks_range_as_df.assert_called_once_with( + symbol="EURUSD", + date_from=datetime(2024, 1, 1, tzinfo=UTC), + date_to=datetime(2024, 2, 1, tzinfo=UTC), + flags=-1, + ) with sqlite3.connect(output) as conn: tables = { row[0] @@ -1311,8 +1096,8 @@ class TestCollectHistory: "SELECT name FROM sqlite_master WHERE type='table'", ).fetchall() } - assert "ticks" in tables - assert "rates" not in tables + assert required_tables <= tables + assert forbidden_table not in tables def test_collect_history_history_fetched_per_symbol( self, @@ -1437,12 +1222,22 @@ class TestCollectHistory: ).fetchall() assert rows == [(16385,)] - def test_collect_history_if_exists_append( + @pytest.mark.parametrize( + ("if_exists", "second_exit_code", "expected_count"), + [ + pytest.param("append", 0, 2, id="append-accumulates-rows"), + pytest.param("fail", 1, 1, id="fail-rejects-existing-table"), + ], + ) + def test_collect_history_if_exists( self, tmp_path: Path, history_client: MagicMock, # noqa: ARG002 + if_exists: str, + second_exit_code: int, + expected_count: int, ) -> None: - """Test that --if-exists=append accumulates rows across runs.""" + """Test --if-exists=append accumulates and --if-exists=fail rejects.""" output = tmp_path / "history.db" common = [ "-o", @@ -1458,68 +1253,12 @@ class TestCollectHistory: "rates", ] first = runner.invoke(app, common) - second = runner.invoke(app, [*common, "--if-exists", "append"]) + second = runner.invoke(app, [*common, "--if-exists", if_exists]) assert first.exit_code == 0, first.output - assert second.exit_code == 0, second.output + assert second.exit_code == second_exit_code, second.output with sqlite3.connect(output) as conn: (count,) = conn.execute("SELECT COUNT(*) FROM rates").fetchone() - assert count == 2 - - def test_collect_history_if_exists_fail( - self, - tmp_path: Path, - history_client: MagicMock, # noqa: ARG002 - ) -> None: - """Test that --if-exists=fail rejects writing into an existing table.""" - output = tmp_path / "history.db" - common = [ - "-o", - str(output), - "collect-history", - "--symbol", - "EURUSD", - "--date-from", - "2024-01-01", - "--date-to", - "2024-02-01", - "--dataset", - "rates", - ] - first = runner.invoke(app, common) - second = runner.invoke(app, [*common, "--if-exists", "fail"]) - assert first.exit_code == 0, first.output - assert second.exit_code != 0 - - def test_collect_history_ticks_default_flags_all( - self, - tmp_path: Path, - history_client: MagicMock, - ) -> None: - """Test that --flags defaults to ALL when --dataset ticks is explicit.""" - output = tmp_path / "history.db" - result = runner.invoke( - app, - [ - "-o", - str(output), - "collect-history", - "--symbol", - "EURUSD", - "--date-from", - "2024-01-01", - "--date-to", - "2024-02-01", - "--dataset", - "ticks", - ], - ) - assert result.exit_code == 0, result.output - history_client.copy_ticks_range_as_df.assert_called_once_with( - symbol="EURUSD", - date_from=datetime(2024, 1, 1, tzinfo=UTC), - date_to=datetime(2024, 2, 1, tzinfo=UTC), - flags=-1, - ) + assert count == expected_count def test_collect_history_with_views( self, @@ -1635,50 +1374,42 @@ class TestCollectHistory: assert order_symbols == [("EURUSD",)] assert deal_symbols == [("EURUSD",)] - def test_collect_history_requires_sqlite_format( + @pytest.mark.parametrize( + ("output_name", "extra_args", "expected_fragment"), + [ + pytest.param( + "history.csv", + ["--symbol", "EURUSD"], + "requires SQLite3", + id="non-sqlite-format", + ), + pytest.param("history.db", [], None, id="missing-symbol"), + ], + ) + def test_collect_history_validation_failures( self, tmp_path: Path, history_client: MagicMock, # noqa: ARG002 + output_name: str, + extra_args: list[str], + expected_fragment: str | None, ) -> None: - """Test that non-SQLite output is rejected.""" - output = tmp_path / "history.csv" - result = runner.invoke( - app, - [ - "-o", - str(output), - "collect-history", - "--symbol", - "EURUSD", - "--date-from", - "2024-01-01", - "--date-to", - "2024-02-01", - ], - ) - assert result.exit_code != 0 - assert "requires SQLite3" in normalize_cli_output(result.output) - - def test_collect_history_requires_symbol( - self, - tmp_path: Path, - history_client: MagicMock, # noqa: ARG002 - ) -> None: - """Test that at least one --symbol is required.""" - output = tmp_path / "history.db" - result = runner.invoke( - app, - [ - "-o", - str(output), - "collect-history", - "--date-from", - "2024-01-01", - "--date-to", - "2024-02-01", - ], - ) + """Test collect-history rejects invalid output format and missing symbols.""" + output = tmp_path / output_name + args = [ + "-o", + str(output), + "collect-history", + "--date-from", + "2024-01-01", + "--date-to", + "2024-02-01", + *extra_args, + ] + result = runner.invoke(app, args) assert result.exit_code != 0 + if expected_fragment is not None: + assert expected_fragment in normalize_cli_output(result.output) def test_collect_history_views_skipped_when_columns_missing( self, @@ -1824,74 +1555,69 @@ class TestGrafanaSchemaCommand: assert result1.exit_code == 0, result1.output assert result2.exit_code == 0, result2.output - def test_grafana_schema_rejects_non_sqlite_output( + +class TestNonSqliteRejection: + """Tests that SQLite-only commands reject non-SQLite output.""" + + @pytest.mark.parametrize( + ("command", "match"), + [ + ("grafana-schema", "grafana-schema requires SQLite3 output"), + ("snapshot", "snapshot requires SQLite3 output"), + ], + ids=["grafana-schema", "snapshot"], + ) + def test_rejects_non_sqlite_output( self, tmp_path: Path, + command: str, + match: str, ) -> None: - """grafana-schema fails when output is not a SQLite3 format.""" + """grafana-schema and snapshot reject non-SQLite output formats.""" result = runner.invoke( app, - ["-o", str(tmp_path / "out.csv"), "grafana-schema"], + ["-o", str(tmp_path / "out.csv"), command], ) assert result.exit_code != 0 - assert "grafana-schema requires SQLite3 output" in result.output + assert match in result.output class TestSnapshotCommand: """Tests for the snapshot CLI command.""" - def test_snapshot_rejects_non_sqlite_output(self, tmp_path: Path) -> None: - """Snapshot fails when output is not a SQLite3 format.""" - result = runner.invoke( - app, - ["-o", str(tmp_path / "out.csv"), "snapshot"], - ) - assert result.exit_code != 0 - assert "snapshot requires SQLite3 output" in result.output - + @pytest.mark.parametrize( + ("extra_args", "expected_symbols"), + [ + pytest.param([], None, id="no-symbol-filter"), + pytest.param( + ["--symbol", "EURUSD", "--symbol", "GBPUSD"], + ["EURUSD", "GBPUSD"], + id="with-symbol-filter", + ), + ], + ) def test_snapshot_delegates_to_update_observability_with_config( self, tmp_path: Path, mocker: MockerFixture, + extra_args: list[str], + expected_symbols: list[str] | None, ) -> None: - """Snapshot calls sdk.update_observability_with_config.""" + """Snapshot calls sdk.update_observability_with_config with forwarded kwargs.""" updater = mocker.patch("mt5cli.cli.sdk.update_observability_with_config") output = tmp_path / "out.db" - result = runner.invoke(app, ["-o", str(output), "snapshot"]) + result = runner.invoke(app, ["-o", str(output), "snapshot", *extra_args]) assert result.exit_code == 0, result.output updater.assert_called_once() kwargs = updater.call_args.kwargs assert kwargs["output"] == output - assert kwargs["symbols"] is None + assert kwargs["symbols"] == expected_symbols assert kwargs["include_account"] is True assert kwargs["include_positions"] is True assert kwargs["include_orders"] is True assert kwargs["include_terminal"] is True assert kwargs["with_grafana_schema"] is False - def test_snapshot_with_symbol_filter( - self, - tmp_path: Path, - mocker: MockerFixture, - ) -> None: - """Snapshot passes symbol list to update_observability_with_config.""" - updater = mocker.patch("mt5cli.cli.sdk.update_observability_with_config") - result = runner.invoke( - app, - [ - "-o", - str(tmp_path / "out.db"), - "snapshot", - "--symbol", - "EURUSD", - "--symbol", - "GBPUSD", - ], - ) - assert result.exit_code == 0, result.output - kwargs = updater.call_args.kwargs - assert kwargs["symbols"] == ["EURUSD", "GBPUSD"] - @pytest.mark.parametrize( ("flag", "kwarg"), [ @@ -1918,75 +1644,40 @@ class TestSnapshotCommand: assert result.exit_code == 0, result.output assert updater.call_args.kwargs[kwarg] is False - def test_snapshot_with_publish_copy( - self, - tmp_path: Path, - mocker: MockerFixture, - ) -> None: - """--publish-copy calls publish_grafana_copy after update_observability.""" + +@pytest.mark.parametrize( + ("command", "patch_update_observability"), + [ + ("snapshot", True), + ("grafana-schema", False), + ], + ids=["snapshot", "grafana-schema"], +) +@pytest.mark.parametrize( + ("use_publish_copy", "expect_called"), + [(True, True), (False, False)], + ids=["with-publish-copy", "no-publish-copy"], +) +def test_publish_copy_option_gates_grafana_copy( + tmp_path: Path, + mocker: MockerFixture, + command: str, + patch_update_observability: bool, + use_publish_copy: bool, + expect_called: bool, +) -> None: + """--publish-copy gates publish_grafana_copy for copy-capable commands.""" + if patch_update_observability: mocker.patch("mt5cli.cli.sdk.update_observability_with_config") - mock_publish = mocker.patch("mt5cli.grafana.publish_grafana_copy") - copy_path = tmp_path / "grafana.db" - result = runner.invoke( - app, - [ - "-o", - str(tmp_path / "out.db"), - "snapshot", - "--publish-copy", - str(copy_path), - ], - ) - assert result.exit_code == 0, result.output + mock_publish = mocker.patch("mt5cli.grafana.publish_grafana_copy") + args = ["-o", str(tmp_path / "out.db"), command] + if use_publish_copy: + args += ["--publish-copy", str(tmp_path / "grafana.db")] + result = runner.invoke(app, args) + assert result.exit_code == 0, result.output + if expect_called: mock_publish.assert_called_once() - - def test_snapshot_no_publish_copy_by_default( - self, - tmp_path: Path, - mocker: MockerFixture, - ) -> None: - """Snapshot does not call publish_grafana_copy without --publish-copy.""" - mocker.patch("mt5cli.cli.sdk.update_observability_with_config") - mock_publish = mocker.patch("mt5cli.grafana.publish_grafana_copy") - result = runner.invoke(app, ["-o", str(tmp_path / "out.db"), "snapshot"]) - assert result.exit_code == 0, result.output - mock_publish.assert_not_called() - - -class TestGrafanaSchemaPublishCopy: - """Tests for grafana-schema --publish-copy option.""" - - def test_grafana_schema_with_publish_copy( - self, - tmp_path: Path, - mocker: MockerFixture, - ) -> None: - """grafana-schema --publish-copy calls publish_grafana_copy.""" - mock_publish = mocker.patch("mt5cli.grafana.publish_grafana_copy") - output = tmp_path / "out.db" - copy_path = tmp_path / "grafana.db" - result = runner.invoke( - app, - [ - "-o", - str(output), - "grafana-schema", - "--publish-copy", - str(copy_path), - ], - ) - assert result.exit_code == 0, result.output - mock_publish.assert_called_once() - - def test_grafana_schema_no_publish_copy_by_default( - self, - tmp_path: Path, - mocker: MockerFixture, - ) -> None: - """grafana-schema does not call publish_grafana_copy by default.""" - mock_publish = mocker.patch("mt5cli.grafana.publish_grafana_copy") - result = runner.invoke(app, ["-o", str(tmp_path / "out.db"), "grafana-schema"]) - assert result.exit_code == 0, result.output + else: mock_publish.assert_not_called() diff --git a/tests/test_contracts.py b/tests/test_contracts.py index 7519466..030e6ac 100644 --- a/tests/test_contracts.py +++ b/tests/test_contracts.py @@ -224,10 +224,18 @@ def test_parse_date_range_rejects_inverted_bounds() -> None: parse_date_range("2024-02-01", "2024-01-01") -def test_recent_window_builds_trailing_bounds() -> None: - """Recent windows end at the provided timestamp.""" +@pytest.mark.parametrize( + "kwargs", + [ + {"hours": 24}, + {"seconds": 3600}, + ], + ids=["hours", "seconds"], +) +def test_recent_window_success_cases(kwargs: dict[str, int]) -> None: + """Recent windows end at the provided timestamp for both duration inputs.""" end = datetime(2024, 1, 2, tzinfo=UTC) - start, resolved_end = recent_window(hours=24, date_to=end) + start, resolved_end = recent_window(date_to=end, **kwargs) assert resolved_end == end assert start < end @@ -339,22 +347,22 @@ def test_ensure_utc_handles_naive_and_aware_datetimes() -> None: assert ensure_utc("2024-01-01T00:00:00+00:00").tzinfo == UTC -def test_recent_window_validation_errors() -> None: +@pytest.mark.parametrize( + ("kwargs", "match"), + [ + ({}, "exactly one"), + ({"hours": 1, "seconds": 1}, "exactly one"), + ({"hours": 0}, "positive"), + ], + ids=["no-duration", "both-hours-and-seconds", "non-positive-duration"], +) +def test_recent_window_validation_errors( + kwargs: dict[str, object], + match: str, +) -> None: """Recent window helpers validate mutually exclusive length arguments.""" - with pytest.raises(ValueError, match="exactly one"): - recent_window() - with pytest.raises(ValueError, match="exactly one"): - recent_window(hours=1, seconds=1) - with pytest.raises(ValueError, match="positive"): - recent_window(hours=0) - - -def test_recent_window_supports_seconds_argument() -> None: - """Recent windows can be built from a seconds-based length.""" - end = datetime(2024, 1, 2, tzinfo=UTC) - start, resolved_end = recent_window(seconds=3600, date_to=end) - assert resolved_end == end - assert start < end + with pytest.raises(ValueError, match=match): + recent_window(**kwargs) # type: ignore[arg-type] def test_parse_date_range_returns_ordered_bounds() -> None: diff --git a/tests/test_examples.py b/tests/test_examples.py index ddfa60c..a25cb2a 100644 --- a/tests/test_examples.py +++ b/tests/test_examples.py @@ -4,72 +4,89 @@ from __future__ import annotations import json from pathlib import Path +from typing import TYPE_CHECKING + +import pytest + +if TYPE_CHECKING: + from collections.abc import Iterator _EXAMPLES_DIR = Path(__file__).parent.parent / "examples" / "grafana" _DASHBOARDS_DIR = _EXAMPLES_DIR / "dashboards" +def _dashboard_json_files() -> list[Path]: + """Return bundled Grafana dashboard JSON files in deterministic order.""" + return sorted(_DASHBOARDS_DIR.glob("*.json")) + + +@pytest.fixture(params=_dashboard_json_files(), ids=lambda path: path.name) +def dashboard_path(request: pytest.FixtureRequest) -> Iterator[Path]: + """Yield one bundled Grafana dashboard JSON file per test case.""" + return request.param + + class TestGrafanaExamples: """Validate structure and content of bundled Grafana example files.""" - def test_dashboard_json_files_are_valid_json(self) -> None: - """All dashboard JSON files parse without error.""" - paths = list(_DASHBOARDS_DIR.glob("*.json")) - assert paths, "No dashboard JSON files found" - for path in paths: - content = path.read_text(encoding="utf-8") - obj = json.loads(content) - assert isinstance(obj, dict), f"{path.name} root must be a JSON object" + def test_dashboard_json_files_are_present(self) -> None: + """At least one dashboard JSON file is bundled.""" + assert _dashboard_json_files(), "No dashboard JSON files found" - def test_dashboard_json_has_no_private_placeholders(self) -> None: + def test_dashboard_json_file_is_valid_json(self, dashboard_path: Path) -> None: + """Each dashboard JSON file parses to a JSON object.""" + content = dashboard_path.read_text(encoding="utf-8") + obj = json.loads(content) + assert isinstance(obj, dict), ( + f"{dashboard_path.name} root must be a JSON object" + ) + + @pytest.mark.parametrize("private_pattern", ["password", "api_key", "apikey"]) + def test_dashboard_json_has_no_private_placeholders( + self, + dashboard_path: Path, + private_pattern: str, + ) -> None: """Dashboard JSON files contain no obvious credential placeholders.""" - private_patterns = ["password", "api_key", "apikey"] - for path in _DASHBOARDS_DIR.glob("*.json"): - content = path.read_text(encoding="utf-8").lower() - for pat in private_patterns: - assert pat not in content, f"{path.name} contains {pat!r}" + content = dashboard_path.read_text(encoding="utf-8").lower() + assert private_pattern not in content, ( + f"{dashboard_path.name} contains {private_pattern!r}" + ) - def test_dashboard_json_uses_grafana_views(self) -> None: - """All dashboard JSON files query grafana_* views.""" - for path in _DASHBOARDS_DIR.glob("*.json"): - content = path.read_text(encoding="utf-8") - assert "grafana_" in content, ( - f"{path.name} must contain queries against grafana_* views" - ) + def test_dashboard_json_uses_grafana_views(self, dashboard_path: Path) -> None: + """Each dashboard JSON file queries grafana_* views.""" + content = dashboard_path.read_text(encoding="utf-8") + assert "grafana_" in content, ( + f"{dashboard_path.name} must contain queries against grafana_* views" + ) - def test_dashboard_json_has_uid(self) -> None: - """All dashboard JSON files have a non-empty uid field.""" - for path in _DASHBOARDS_DIR.glob("*.json"): - obj = json.loads(path.read_text(encoding="utf-8")) - assert obj.get("uid"), f"{path.name} must have a uid" - - def test_dashboard_json_has_title(self) -> None: - """All dashboard JSON files have a non-empty title field.""" - for path in _DASHBOARDS_DIR.glob("*.json"): - obj = json.loads(path.read_text(encoding="utf-8")) - assert obj.get("title"), f"{path.name} must have a title" + @pytest.mark.parametrize("field", ["uid", "title"]) + def test_dashboard_json_has_required_field( + self, + dashboard_path: Path, + field: str, + ) -> None: + """Each dashboard JSON file has a non-empty uid and title field.""" + obj = json.loads(dashboard_path.read_text(encoding="utf-8")) + assert obj.get(field), f"{dashboard_path.name} must have a {field}" def test_expected_dashboards_present(self) -> None: """The three expected dashboard files are present.""" - names = {p.name for p in _DASHBOARDS_DIR.glob("*.json")} + names = {p.name for p in _dashboard_json_files()} assert "mt5cli-overview.json" in names assert "mt5cli-trades.json" in names assert "mt5cli-market.json" in names - def test_readme_exists(self) -> None: - """examples/grafana/README.md is present.""" - assert (_EXAMPLES_DIR / "README.md").is_file() - - def test_compose_file_exists(self) -> None: - """examples/grafana/compose.yml is present.""" - assert (_EXAMPLES_DIR / "compose.yml").is_file() - - def test_datasource_provisioning_exists(self) -> None: - """Datasource provisioning YAML is present.""" - assert ( - _EXAMPLES_DIR / "provisioning" / "datasources" / "mt5cli-sqlite.yml" - ).is_file() - - def test_dashboard_provisioning_exists(self) -> None: - """Dashboard provisioning YAML is present.""" - assert (_EXAMPLES_DIR / "provisioning" / "dashboards" / "mt5cli.yml").is_file() + @pytest.mark.parametrize( + "relative_path", + [ + "README.md", + "compose.yml", + "provisioning/datasources/mt5cli-sqlite.yml", + "provisioning/dashboards/mt5cli.yml", + ], + ids=["readme", "compose", "datasource-provisioning", "dashboard-provisioning"], + ) + def test_expected_file_exists(self, relative_path: str) -> None: + """Bundled Grafana example support files are present.""" + assert (_EXAMPLES_DIR / relative_path).is_file() diff --git a/tests/test_grafana.py b/tests/test_grafana.py index d84c2a6..609def1 100644 --- a/tests/test_grafana.py +++ b/tests/test_grafana.py @@ -12,7 +12,7 @@ import pandas as pd import pytest if TYPE_CHECKING: - from collections.abc import Iterator + from collections.abc import Callable, Iterator from mt5cli.grafana import ( _build_snapshot_view, # type: ignore[reportPrivateUsage] @@ -75,6 +75,14 @@ def _make_history_deals_minimal(conn: sqlite3.Connection) -> None: ) +def _make_history_deals_symbol_pnl_minimal(conn: sqlite3.Connection) -> None: + """history_deals with entry but without volume and price columns.""" + conn.execute( + "CREATE TABLE history_deals" + " (time TEXT, symbol TEXT, profit REAL, type INTEGER, entry INTEGER)" + ) + + def _make_history_orders_table(conn: sqlite3.Connection) -> None: conn.execute( "CREATE TABLE history_orders" @@ -82,6 +90,9 @@ def _make_history_orders_table(conn: sqlite3.Connection) -> None: ) +_TIMESTAMP_TIME_SETUP: pd.Timestamp = pd.Timestamp("2024-01-15 10:30:00", tz="UTC") + + # --------------------------------------------------------------------------- # TestSnapshotTables # --------------------------------------------------------------------------- @@ -204,169 +215,97 @@ class TestGrafanaViews: create_grafana_views(conn) assert "grafana_rates" not in _get_names(conn, "view") - def test_grafana_rates_skipped_when_required_cols_missing( + @pytest.mark.parametrize( + ("ddl", "view_name"), + [ + ("CREATE TABLE rates (open REAL)", "grafana_rates"), + ("CREATE TABLE ticks (bid REAL)", "grafana_ticks"), + ("CREATE TABLE history_deals (symbol TEXT)", "grafana_history_deals"), + ("CREATE TABLE history_orders (symbol TEXT)", "grafana_history_orders"), + ("CREATE TABLE history_deals (symbol TEXT)", "grafana_trade_deals"), + ("CREATE TABLE history_deals (symbol TEXT)", "grafana_cash_events"), + ( + "CREATE TABLE history_deals (time TEXT, type INTEGER)", + "grafana_realized_pnl", + ), + ( + ( + "CREATE TABLE history_deals" + " (time TEXT, symbol TEXT, profit REAL, type INTEGER)" + ), + "grafana_realized_pnl", + ), + ( + "CREATE TABLE history_deals (time TEXT, type INTEGER)", + "grafana_symbol_pnl", + ), + ("CREATE TABLE history_deals (time TEXT)", "grafana_trade_stats"), + ], + ids=[ + "rates-cols", + "ticks-cols", + "history_deals-time", + "history_orders-time_setup", + "trade_deals-cols", + "cash_events-cols", + "realized_pnl-cols", + "realized_pnl-entry", + "symbol_pnl-cols", + "trade_stats-cols", + ], + ) + def test_grafana_view_skipped_when_required_cols_missing( self, conn: sqlite3.Connection, caplog: pytest.LogCaptureFixture, + ddl: str, + view_name: str, ) -> None: - """grafana_rates is skipped when rates table lacks required columns.""" - conn.execute("CREATE TABLE rates (open REAL)") + """A grafana view is skipped (with warning) when source columns are missing.""" + conn.execute(ddl) with caplog.at_level(logging.WARNING, logger="mt5cli.grafana"): create_grafana_views(conn) - assert "grafana_rates" not in _get_names(conn, "view") - assert "Skipping grafana_rates" in caplog.text + assert view_name not in _get_names(conn, "view") + assert f"Skipping {view_name}" in caplog.text - def test_grafana_ticks_skipped_when_cols_missing( + @pytest.mark.parametrize( + ("setup_deals", "optional_cols"), + [ + pytest.param( + _make_history_deals_symbol_pnl_minimal, set[str](), id="minimal" + ), + pytest.param( + _make_history_deals_full, + {"volume", "price"}, + id="full", + ), + ], + ) + def test_grafana_symbol_pnl_schema( self, conn: sqlite3.Connection, - caplog: pytest.LogCaptureFixture, + setup_deals: Callable[[sqlite3.Connection], None], + optional_cols: set[str], ) -> None: - """grafana_ticks is skipped when ticks table lacks required columns.""" - conn.execute("CREATE TABLE ticks (bid REAL)") - with caplog.at_level(logging.WARNING, logger="mt5cli.grafana"): - create_grafana_views(conn) - assert "grafana_ticks" not in _get_names(conn, "view") - assert "Skipping grafana_ticks" in caplog.text - - def test_grafana_history_deals_skipped_when_time_missing( - self, - conn: sqlite3.Connection, - caplog: pytest.LogCaptureFixture, - ) -> None: - """grafana_history_deals is skipped when history_deals.time is missing.""" - conn.execute("CREATE TABLE history_deals (symbol TEXT)") - with caplog.at_level(logging.WARNING, logger="mt5cli.grafana"): - create_grafana_views(conn) - assert "grafana_history_deals" not in _get_names(conn, "view") - assert "Skipping grafana_history_deals" in caplog.text - - def test_grafana_history_orders_skipped_when_time_setup_missing( - self, - conn: sqlite3.Connection, - caplog: pytest.LogCaptureFixture, - ) -> None: - """grafana_history_orders is skipped when time_setup is absent.""" - conn.execute("CREATE TABLE history_orders (symbol TEXT)") - with caplog.at_level(logging.WARNING, logger="mt5cli.grafana"): - create_grafana_views(conn) - assert "grafana_history_orders" not in _get_names(conn, "view") - assert "Skipping grafana_history_orders" in caplog.text - - def test_grafana_trade_deals_skipped_when_cols_missing( - self, - conn: sqlite3.Connection, - caplog: pytest.LogCaptureFixture, - ) -> None: - """grafana_trade_deals is skipped when history_deals missing time/type.""" - conn.execute("CREATE TABLE history_deals (symbol TEXT)") - with caplog.at_level(logging.WARNING, logger="mt5cli.grafana"): - create_grafana_views(conn) - assert "grafana_trade_deals" not in _get_names(conn, "view") - - def test_grafana_cash_events_skipped_when_cols_missing( - self, - conn: sqlite3.Connection, - caplog: pytest.LogCaptureFixture, - ) -> None: - """grafana_cash_events is skipped when history_deals missing time/type.""" - conn.execute("CREATE TABLE history_deals (symbol TEXT)") - with caplog.at_level(logging.WARNING, logger="mt5cli.grafana"): - create_grafana_views(conn) - assert "grafana_cash_events" not in _get_names(conn, "view") - - def test_grafana_realized_pnl_skipped_when_cols_missing( - self, - conn: sqlite3.Connection, - caplog: pytest.LogCaptureFixture, - ) -> None: - """grafana_realized_pnl is skipped when history_deals missing required cols.""" - conn.execute("CREATE TABLE history_deals (time TEXT, type INTEGER)") - with caplog.at_level(logging.WARNING, logger="mt5cli.grafana"): - create_grafana_views(conn) - assert "grafana_realized_pnl" not in _get_names(conn, "view") - assert "Skipping grafana_realized_pnl" in caplog.text - - def test_grafana_realized_pnl_skipped_when_entry_missing( - self, - conn: sqlite3.Connection, - caplog: pytest.LogCaptureFixture, - ) -> None: - """grafana_realized_pnl is skipped when entry column is absent.""" - _make_history_deals_minimal(conn) - with caplog.at_level(logging.WARNING, logger="mt5cli.grafana"): - create_grafana_views(conn) - assert "grafana_realized_pnl" not in _get_names(conn, "view") - assert "Skipping grafana_realized_pnl" in caplog.text - - def test_grafana_symbol_pnl_skipped_when_required_cols_missing( - self, - conn: sqlite3.Connection, - caplog: pytest.LogCaptureFixture, - ) -> None: - """grafana_symbol_pnl is skipped when required columns are absent.""" - conn.execute("CREATE TABLE history_deals (time TEXT, type INTEGER)") - with caplog.at_level(logging.WARNING, logger="mt5cli.grafana"): - create_grafana_views(conn) - assert "grafana_symbol_pnl" not in _get_names(conn, "view") - assert "Skipping grafana_symbol_pnl" in caplog.text - - def test_grafana_symbol_pnl_without_volume_and_price( - self, - conn: sqlite3.Connection, - ) -> None: - """grafana_symbol_pnl is created with only required columns.""" - conn.execute( - "CREATE TABLE history_deals" - " (time TEXT, symbol TEXT, profit REAL, type INTEGER, entry INTEGER)" - ) + """grafana_symbol_pnl is created and includes optional columns when present.""" + setup_deals(conn) create_grafana_views(conn) assert "grafana_symbol_pnl" in _get_names(conn, "view") - - def test_grafana_symbol_pnl_with_volume_and_price( - self, - conn: sqlite3.Connection, - ) -> None: - """grafana_symbol_pnl includes volume and price columns when present.""" - _make_history_deals_full(conn) - create_grafana_views(conn) - assert "grafana_symbol_pnl" in _get_names(conn, "view") - # View columns include volume and price cols = {row[1] for row in conn.execute("PRAGMA table_info(grafana_symbol_pnl)")} - assert "volume" in cols - assert "price" in cols + assert optional_cols.issubset(cols) - def test_grafana_trade_stats_skipped_when_cols_missing( - self, - conn: sqlite3.Connection, - caplog: pytest.LogCaptureFixture, - ) -> None: - """grafana_trade_stats is skipped when history_deals missing required cols.""" - conn.execute("CREATE TABLE history_deals (time TEXT)") - with caplog.at_level(logging.WARNING, logger="mt5cli.grafana"): - create_grafana_views(conn) - assert "grafana_trade_stats" not in _get_names(conn, "view") - assert "Skipping grafana_trade_stats" in caplog.text - - def test_grafana_trade_stats_without_entry_col( + @pytest.mark.parametrize( + "setup_deals", + [_make_history_deals_minimal, _make_history_deals_full], + ids=["minimal", "full"], + ) + def test_grafana_trade_stats_static_summary( self, conn: sqlite3.Connection, + setup_deals: Callable[[sqlite3.Connection], None], ) -> None: """grafana_trade_stats is a static summary view with no time column.""" - _make_history_deals_minimal(conn) - create_grafana_views(conn) - assert "grafana_trade_stats" in _get_names(conn, "view") - cols = { - row[1] for row in conn.execute("PRAGMA table_info(grafana_trade_stats)") - } - assert "time" not in cols - assert "symbol" in cols - - def test_grafana_trade_stats_with_entry_col( - self, - conn: sqlite3.Connection, - ) -> None: - """grafana_trade_stats is a static summary view with no time column.""" - _make_history_deals_full(conn) + setup_deals(conn) create_grafana_views(conn) assert "grafana_trade_stats" in _get_names(conn, "view") cols = { @@ -405,116 +344,126 @@ class TestGrafanaViews: assert "time" in cols assert "run_id" in cols - def test_build_snapshot_view_skips_when_snapshot_runs_missing( - self, - conn: sqlite3.Connection, - ) -> None: - """_build_snapshot_view skips view when snapshot_runs has wrong columns.""" - conn.execute("CREATE TABLE only_run (run_id INTEGER NOT NULL)") - conn.execute("CREATE TABLE snapshot_runs (foo TEXT)") - _build_snapshot_view(conn, "test_view", "only_run") - views = _get_names(conn, "view") - assert "test_view" not in views - - def test_build_snapshot_view_skips_when_run_id_col_missing( + @pytest.mark.parametrize( + ("use_snapshot_tables", "table_ddl", "table_name", "log_fragment"), + [ + pytest.param( + False, + "CREATE TABLE only_run (run_id INTEGER NOT NULL)", + "only_run", + "snapshot_runs missing required columns", + id="snapshot-runs-wrong-columns", + ), + pytest.param( + True, + "CREATE TABLE no_run_id (symbol TEXT)", + "no_run_id", + "missing run_id column", + id="table-missing-run-id", + ), + ], + ) + def test_build_snapshot_view_skips_negative_cases( self, conn: sqlite3.Connection, caplog: pytest.LogCaptureFixture, + use_snapshot_tables: bool, + table_ddl: str, + table_name: str, + log_fragment: str, ) -> None: - """_build_snapshot_view skips view when the table lacks run_id.""" - create_snapshot_tables(conn) - conn.execute("CREATE TABLE no_run_id (symbol TEXT)") + """_build_snapshot_view skips view creation for invalid table/run metadata.""" + if use_snapshot_tables: + create_snapshot_tables(conn) + else: + conn.execute("CREATE TABLE snapshot_runs (foo TEXT)") + conn.execute(table_ddl) with caplog.at_level(logging.WARNING, logger="mt5cli.grafana"): - _build_snapshot_view(conn, "test_view", "no_run_id") + _build_snapshot_view(conn, "test_view", table_name) assert "test_view" not in _get_names(conn, "view") - assert "missing run_id column" in caplog.text + assert log_fragment in caplog.text - def test_snapshot_view_excludes_failed_run_rows( + @pytest.mark.parametrize( + ("started_at", "status", "message", "keep_row"), + [ + pytest.param( + 1000, + "error", + "terminal offline", + False, + id="excludes-failed-run-rows", + ), + pytest.param(2000, "ok", None, True, id="includes-ok-run-rows"), + ], + ) + def test_snapshot_view_filters_rows_by_run_status( self, conn: sqlite3.Connection, + started_at: int, + status: str, + message: str | None, + keep_row: bool, ) -> None: - """Snapshot views hide rows from failed runs.""" + """Snapshot views expose rows only from successful runs.""" create_snapshot_tables(conn) - run_id = start_snapshot_run(conn, 1000) + run_id = start_snapshot_run(conn, started_at) conn.execute( "INSERT INTO account_snapshots" " (run_id, login, balance, equity, margin, margin_free, profit)" " VALUES (?, 12345, 10000.0, 9800.0, 200.0, 9600.0, -200.0)", (run_id,), ) - record_snapshot_run(conn, run_id, "error", "terminal offline") - create_grafana_views(conn) - rows = conn.execute("SELECT * FROM grafana_account_snapshots").fetchall() - assert rows == [] - - def test_snapshot_view_includes_ok_run_rows( - self, - conn: sqlite3.Connection, - ) -> None: - """Snapshot views show rows from successful runs and expose run_id.""" - create_snapshot_tables(conn) - run_id = start_snapshot_run(conn, 2000) - conn.execute( - "INSERT INTO account_snapshots" - " (run_id, login, balance, equity, margin, margin_free, profit)" - " VALUES (?, 12345, 10000.0, 9800.0, 200.0, 9600.0, -200.0)", - (run_id,), - ) - record_snapshot_run(conn, run_id, "ok") + record_snapshot_run(conn, run_id, status, message) create_grafana_views(conn) rows = conn.execute( "SELECT time, run_id, login FROM grafana_account_snapshots" ).fetchall() - assert rows == [(2000, run_id, 12345)] - cols = { - row[1] - for row in conn.execute("PRAGMA table_info(grafana_account_snapshots)") - } - assert "run_id" in cols + if keep_row: + assert rows == [(started_at, run_id, 12345)] + cols = { + row[1] + for row in conn.execute("PRAGMA table_info(grafana_account_snapshots)") + } + assert "run_id" in cols + else: + assert rows == [] - def test_snapshot_view_same_second_ok_and_error_no_cross_contamination( + @pytest.mark.parametrize( + ("observed_at", "runs", "expected_rows"), + [ + pytest.param( + 3000, + [("error", 99), ("ok", 12345)], + [(12345,)], + id="same-second-error-and-ok-exposes-only-ok-row", + ), + pytest.param( + 4000, + [("ok", 1), ("ok", 2)], + [(1,), (2,)], + id="same-second-two-ok-runs-no-duplication", + ), + ], + ) + def test_snapshot_view_same_second_runs( self, conn: sqlite3.Connection, + observed_at: int, + runs: list[tuple[str, int]], + expected_rows: list[tuple[int]], ) -> None: - """An ok and error run sharing observed_at expose only the ok run's rows.""" + """Snapshot views join same-second rows by run_id.""" create_snapshot_tables(conn) - run_err = start_snapshot_run(conn, 3000) - conn.execute( - "INSERT INTO account_snapshots (run_id, login) VALUES (?, 99)", - (run_err,), - ) - record_snapshot_run(conn, run_err, "error") - run_ok = start_snapshot_run(conn, 3000) - conn.execute( - "INSERT INTO account_snapshots (run_id, login) VALUES (?, 12345)", - (run_ok,), - ) - record_snapshot_run(conn, run_ok, "ok") + for status, login in runs: + run_id = start_snapshot_run(conn, observed_at) + conn.execute( + "INSERT INTO account_snapshots (run_id, login) VALUES (?, ?)", + (run_id, login), + ) + record_snapshot_run(conn, run_id, status) create_grafana_views(conn) rows = conn.execute("SELECT login FROM grafana_account_snapshots").fetchall() - assert rows == [(12345,)] - - def test_snapshot_view_two_ok_runs_same_second_no_duplication( - self, - conn: sqlite3.Connection, - ) -> None: - """Two ok runs sharing observed_at each produce exactly one row in the view.""" - create_snapshot_tables(conn) - run1 = start_snapshot_run(conn, 4000) - conn.execute( - "INSERT INTO account_snapshots (run_id, login) VALUES (?, 1)", - (run1,), - ) - record_snapshot_run(conn, run1, "ok") - run2 = start_snapshot_run(conn, 4000) - conn.execute( - "INSERT INTO account_snapshots (run_id, login) VALUES (?, 2)", - (run2,), - ) - record_snapshot_run(conn, run2, "ok") - create_grafana_views(conn) - rows = conn.execute("SELECT login FROM grafana_account_snapshots").fetchall() - assert len(rows) == 2 + assert rows == expected_rows # --------------------------------------------------------------------------- @@ -569,45 +518,32 @@ class TestGrafanaIndexes: assert "idx_order_snapshots_time_symbol" not in indexes assert "idx_snapshot_runs_time_status" not in indexes - def test_rates_index_skipped_when_cols_missing( + @pytest.mark.parametrize( + ("ddl", "index_name"), + [ + ("CREATE TABLE rates (open REAL)", "idx_rates_time_symbol_timeframe"), + ("CREATE TABLE ticks (bid REAL)", "idx_ticks_time_symbol"), + ( + "CREATE TABLE history_deals (ticket INTEGER)", + "idx_history_deals_time_symbol", + ), + ( + "CREATE TABLE history_orders (ticket INTEGER)", + "idx_history_orders_time_setup_symbol", + ), + ], + ids=["rates", "ticks", "deals", "orders"], + ) + def test_index_skipped_when_cols_missing( self, conn: sqlite3.Connection, + ddl: str, + index_name: str, ) -> None: - """Rates index is skipped when required columns are absent.""" - conn.execute("CREATE TABLE rates (open REAL)") + """An index is skipped when the source table lacks required columns.""" + conn.execute(ddl) create_grafana_indexes(conn) - indexes = _get_names(conn, "index") - assert "idx_rates_time_symbol_timeframe" not in indexes - - def test_ticks_index_skipped_when_cols_missing( - self, - conn: sqlite3.Connection, - ) -> None: - """Ticks index is skipped when required columns are absent.""" - conn.execute("CREATE TABLE ticks (bid REAL)") - create_grafana_indexes(conn) - indexes = _get_names(conn, "index") - assert "idx_ticks_time_symbol" not in indexes - - def test_deals_indexes_skipped_when_cols_missing( - self, - conn: sqlite3.Connection, - ) -> None: - """history_deals indexes are skipped when required columns are absent.""" - conn.execute("CREATE TABLE history_deals (ticket INTEGER)") - create_grafana_indexes(conn) - indexes = _get_names(conn, "index") - assert "idx_history_deals_time_symbol" not in indexes - - def test_orders_index_skipped_when_cols_missing( - self, - conn: sqlite3.Connection, - ) -> None: - """history_orders index is skipped when required columns are absent.""" - conn.execute("CREATE TABLE history_orders (ticket INTEGER)") - create_grafana_indexes(conn) - indexes = _get_names(conn, "index") - assert "idx_history_orders_time_setup_symbol" not in indexes + assert index_name not in _get_names(conn, "index") def test_snapshot_indexes_skipped_when_cols_missing( self, @@ -678,25 +614,56 @@ class TestSnapshotInserts: """Create snapshot tables before each insert test.""" create_snapshot_tables(conn) - def test_insert_account_snapshot(self, conn: sqlite3.Connection) -> None: - """insert_account_snapshot appends a row with correct values.""" + @pytest.mark.parametrize( + ("insert_func", "row", "select_sql", "expected"), + [ + ( + insert_account_snapshot, + { + "login": 12345, + "currency": "USD", + "balance": 10000.0, + "equity": 9800.0, + "margin": 200.0, + "margin_free": 9800.0, + "margin_level": 4900.0, + "profit": -200.0, + "leverage": 100, + }, + "SELECT login, currency, balance FROM account_snapshots", + (12345, "USD", 10000.0), + ), + ( + insert_terminal_snapshot, + { + "name": "MetaTrader 5", + "connected": 1, + "community_account": 0, + "trade_allowed": 1, + "trade_expert": 1, + "path": "/mt5", + "company": "Broker", + "language": "en", + }, + "SELECT name, connected FROM terminal_snapshots", + ("MetaTrader 5", 1), + ), + ], + ids=["account", "terminal"], + ) + def test_insert_single_snapshot( + self, + conn: sqlite3.Connection, + insert_func: Callable[[sqlite3.Connection, int, dict[str, object]], None], + row: dict[str, object], + select_sql: str, + expected: tuple[object, ...], + ) -> None: + """insert_account_snapshot and insert_terminal_snapshot append one row.""" run_id = start_snapshot_run(conn, 1700000000) - row: dict[str, object] = { - "login": 12345, - "currency": "USD", - "balance": 10000.0, - "equity": 9800.0, - "margin": 200.0, - "margin_free": 9800.0, - "margin_level": 4900.0, - "profit": -200.0, - "leverage": 100, - } - insert_account_snapshot(conn, run_id, row) - result = conn.execute( - "SELECT login, currency, balance FROM account_snapshots" - ).fetchone() - assert result == (12345, "USD", 10000.0) + insert_func(conn, run_id, row) + result = conn.execute(select_sql).fetchone() + assert result == expected def test_insert_account_snapshot_partial_row( self, @@ -710,105 +677,91 @@ class TestSnapshotInserts: ).fetchone() assert result == (1, None) - def test_insert_position_snapshots_with_rows( + @pytest.mark.parametrize( + ("insert_func", "table", "rows", "expected_count"), + [ + ( + insert_position_snapshots, + "position_snapshots", + [ + {"ticket": 1, "symbol": "EURUSD", "volume": 0.1, "profit": 10.0}, + {"ticket": 2, "symbol": "GBPUSD", "volume": 0.2, "profit": -5.0}, + ], + 2, + ), + ( + insert_position_snapshots, + "position_snapshots", + [], + 0, + ), + ( + insert_order_snapshots, + "order_snapshots", + [ + { + "ticket": 10, + "symbol": "EURUSD", + "type": 2, + "volume_current": 0.1, + }, + ], + 1, + ), + ( + insert_order_snapshots, + "order_snapshots", + [], + 0, + ), + ], + ids=[ + "positions-with-rows", + "positions-empty-noop", + "orders-with-rows", + "orders-empty-noop", + ], + ) + def test_insert_snapshot_rows( self, conn: sqlite3.Connection, + insert_func: Callable[ + [sqlite3.Connection, int, int | None, list[dict[str, object]]], + None, + ], + table: str, + rows: list[dict[str, object]], + expected_count: int, ) -> None: - """insert_position_snapshots appends each position row.""" + """insert_*_snapshots appends each row and is a no-op when empty.""" run_id = start_snapshot_run(conn, 1700000000) - rows: list[dict[str, object]] = [ - {"ticket": 1, "symbol": "EURUSD", "volume": 0.1, "profit": 10.0}, - {"ticket": 2, "symbol": "GBPUSD", "volume": 0.2, "profit": -5.0}, - ] - insert_position_snapshots(conn, run_id, 12345, rows) - count = conn.execute("SELECT COUNT(*) FROM position_snapshots").fetchone()[0] - assert count == 2 + insert_func(conn, run_id, 12345, rows) + count = conn.execute( + f"SELECT COUNT(*) FROM {table}" # noqa: S608 + ).fetchone()[0] + assert count == expected_count - def test_insert_position_snapshots_noop_when_empty( + @pytest.mark.parametrize( + ("time_setup", "expected_stored"), + [ + (_TIMESTAMP_TIME_SETUP, int(_TIMESTAMP_TIME_SETUP.timestamp())), + (1705314600, 1705314600), + ("not_a_time", None), + ], + ids=["timestamp", "int", "unknown-string"], + ) + def test_insert_order_snapshots_normalizes_time_setup( self, conn: sqlite3.Connection, + time_setup: object, + expected_stored: int | None, ) -> None: - """insert_position_snapshots is a no-op when rows is empty.""" + """insert_order_snapshots stores epoch int, int as-is, or None for unknown.""" run_id = start_snapshot_run(conn, 1700000000) - insert_position_snapshots(conn, run_id, 12345, []) - count = conn.execute("SELECT COUNT(*) FROM position_snapshots").fetchone()[0] - assert count == 0 - - def test_insert_order_snapshots_with_rows( - self, - conn: sqlite3.Connection, - ) -> None: - """insert_order_snapshots appends each order row.""" - run_id = start_snapshot_run(conn, 1700000000) - rows: list[dict[str, object]] = [ - {"ticket": 10, "symbol": "EURUSD", "type": 2, "volume_current": 0.1}, - ] - insert_order_snapshots(conn, run_id, 12345, rows) - count = conn.execute("SELECT COUNT(*) FROM order_snapshots").fetchone()[0] - assert count == 1 - - def test_insert_order_snapshots_noop_when_empty( - self, - conn: sqlite3.Connection, - ) -> None: - """insert_order_snapshots is a no-op when rows is empty.""" - run_id = start_snapshot_run(conn, 1700000000) - insert_order_snapshots(conn, run_id, 12345, []) - count = conn.execute("SELECT COUNT(*) FROM order_snapshots").fetchone()[0] - assert count == 0 - - def test_insert_order_snapshots_normalizes_timestamp_time_setup( - self, - conn: sqlite3.Connection, - ) -> None: - """insert_order_snapshots converts pd.Timestamp time_setup to epoch int.""" - run_id = start_snapshot_run(conn, 1700000000) - ts = pd.Timestamp("2024-01-15 10:30:00", tz="UTC") - rows: list[dict[str, object]] = [{"ticket": 10, "time_setup": ts}] + rows: list[dict[str, object]] = [{"ticket": 10, "time_setup": time_setup}] insert_order_snapshots(conn, run_id, 12345, rows) stored = conn.execute("SELECT time_setup FROM order_snapshots").fetchone()[0] - assert stored == int(ts.timestamp()) - - def test_insert_order_snapshots_stores_int_time_setup( - self, - conn: sqlite3.Connection, - ) -> None: - """insert_order_snapshots stores an integer time_setup as-is.""" - run_id = start_snapshot_run(conn, 1700000000) - rows: list[dict[str, object]] = [{"ticket": 10, "time_setup": 1705314600}] - insert_order_snapshots(conn, run_id, 12345, rows) - stored = conn.execute("SELECT time_setup FROM order_snapshots").fetchone()[0] - assert stored == 1705314600 - - def test_insert_order_snapshots_stores_null_for_unknown_time_setup_type( - self, - conn: sqlite3.Connection, - ) -> None: - """insert_order_snapshots stores NULL for an unrecognized time_setup type.""" - run_id = start_snapshot_run(conn, 1700000000) - rows: list[dict[str, object]] = [{"ticket": 10, "time_setup": "not_a_time"}] - insert_order_snapshots(conn, run_id, 12345, rows) - stored = conn.execute("SELECT time_setup FROM order_snapshots").fetchone()[0] - assert stored is None - - def test_insert_terminal_snapshot(self, conn: sqlite3.Connection) -> None: - """insert_terminal_snapshot appends a terminal info row.""" - run_id = start_snapshot_run(conn, 1700000000) - row: dict[str, object] = { - "name": "MetaTrader 5", - "connected": 1, - "community_account": 0, - "trade_allowed": 1, - "trade_expert": 1, - "path": "/mt5", - "company": "Broker", - "language": "en", - } - insert_terminal_snapshot(conn, run_id, row) - result = conn.execute( - "SELECT name, connected FROM terminal_snapshots" - ).fetchone() - assert result == ("MetaTrader 5", 1) + assert stored == expected_stored def test_start_snapshot_run_returns_incrementing_ids( self, @@ -819,25 +772,30 @@ class TestSnapshotInserts: run2 = start_snapshot_run(conn, 1700000000) assert run1 != run2 - def test_record_snapshot_run_with_detail( + @pytest.mark.parametrize( + ("status", "detail", "expected"), + [ + pytest.param( + "error", + "RuntimeError: boom", + ("error", "RuntimeError: boom"), + id="with-detail", + ), + pytest.param("ok", None, ("ok", None), id="without-detail"), + ], + ) + def test_record_snapshot_run( self, conn: sqlite3.Connection, + status: str, + detail: str | None, + expected: tuple[str, str | None], ) -> None: - """record_snapshot_run stores status and detail text.""" + """record_snapshot_run stores status and optional detail text.""" run_id = start_snapshot_run(conn, 1700000000) - record_snapshot_run(conn, run_id, "error", "RuntimeError: boom") + record_snapshot_run(conn, run_id, status, detail) row = conn.execute("SELECT status, detail FROM snapshot_runs").fetchone() - assert row == ("error", "RuntimeError: boom") - - def test_record_snapshot_run_without_detail( - self, - conn: sqlite3.Connection, - ) -> None: - """record_snapshot_run stores None for detail when omitted.""" - run_id = start_snapshot_run(conn, 1700000000) - record_snapshot_run(conn, run_id, "ok") - row = conn.execute("SELECT status, detail FROM snapshot_runs").fetchone() - assert row == ("ok", None) + assert row == expected # --------------------------------------------------------------------------- @@ -859,23 +817,40 @@ def _make_source_db(path: Path) -> None: class TestPublishGrafanaCopy: """Tests for publish_grafana_copy.""" - def test_publish_to_fresh_target(self, tmp_path: Path) -> None: - """publish_grafana_copy creates the target file.""" + @pytest.mark.parametrize( + ("target_rel", "stale_content", "required_tables"), + [ + pytest.param( + Path("out") / "grafana.db", + None, + frozenset({"snapshot_runs", "account_snapshots"}), + id="fresh-target", + ), + pytest.param( + Path("grafana.db"), + b"stale", + frozenset({"snapshot_runs"}), + id="overwrite-stale-target", + ), + ], + ) + def test_publish_creates_valid_sqlite_target( + self, + tmp_path: Path, + target_rel: Path, + stale_content: bytes | None, + required_tables: frozenset[str], + ) -> None: + """publish_grafana_copy creates or replaces a valid SQLite target.""" source = tmp_path / "src.db" - target = tmp_path / "out" / "grafana.db" + target = tmp_path / target_rel _make_source_db(source) + if stale_content is not None: + target.write_bytes(stale_content) result = publish_grafana_copy(source, target) assert target.exists() + assert isinstance(result, Path) assert result == target.resolve() - - def test_overwrite_existing_target(self, tmp_path: Path) -> None: - """publish_grafana_copy replaces an existing target without error.""" - source = tmp_path / "src.db" - target = tmp_path / "grafana.db" - _make_source_db(source) - target.write_bytes(b"stale") - publish_grafana_copy(source, target) - # Target must now be a valid SQLite file from source with sqlite3.connect(target) as conn: tables = { row[0] @@ -883,22 +858,7 @@ class TestPublishGrafanaCopy: "SELECT name FROM sqlite_master WHERE type='table'" ).fetchall() } - assert "snapshot_runs" in tables - - def test_target_contains_source_tables(self, tmp_path: Path) -> None: - """Published target contains the same tables as the source.""" - source = tmp_path / "src.db" - target = tmp_path / "grafana.db" - _make_source_db(source) - publish_grafana_copy(source, target) - with sqlite3.connect(target) as conn: - tables = { - row[0] - for row in conn.execute( - "SELECT name FROM sqlite_master WHERE type='table'" - ).fetchall() - } - assert {"snapshot_runs", "account_snapshots"}.issubset(tables) + assert required_tables <= tables def test_target_can_be_opened_readonly(self, tmp_path: Path) -> None: """Published target can be opened with uri=True in read-only mode.""" @@ -956,14 +916,6 @@ class TestPublishGrafanaCopy: tmp_files = list(tmp_path.glob("grafana.db.*.tmp")) assert not tmp_files, "Temp file should be cleaned up on failure" - def test_returns_path_object(self, tmp_path: Path) -> None: - """publish_grafana_copy returns a Path instance.""" - source = tmp_path / "src.db" - target = tmp_path / "grafana.db" - _make_source_db(source) - result = publish_grafana_copy(source, target) - assert isinstance(result, Path) - def test_fresh_target_has_readable_permissions(self, tmp_path: Path) -> None: """Published copy is readable by the owner.""" import stat as _stat # noqa: PLC0415 diff --git a/tests/test_history.py b/tests/test_history.py index 8bc24b3..244668d 100644 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -5,7 +5,7 @@ from __future__ import annotations import logging import sqlite3 from datetime import UTC, datetime -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, cast from unittest.mock import MagicMock import pandas as pd @@ -13,6 +13,7 @@ import pytest from pytest_mock import MockerFixture # noqa: TC002 if TYPE_CHECKING: + from collections.abc import Callable from pathlib import Path from pdmt5 import TIMEFRAME_MAP @@ -64,6 +65,36 @@ from mt5cli.history import ( from mt5cli.utils import Dataset, IfExists +def _resolve_rate_view_single( + conn_or_path: sqlite3.Connection | Path | str | None, + *, + granularity: str = "M1", + require_existing: bool = False, +) -> str: + """Resolve a single EURUSD rate view name via resolve_rate_view_name.""" + return resolve_rate_view_name( + conn_or_path, + "EURUSD", + granularity, + require_existing=require_existing, + ) + + +def _resolve_rate_view_batch( + conn_or_path: sqlite3.Connection | Path | str | None, + *, + granularity: str = "M1", + require_existing: bool = False, +) -> list[str]: + """Resolve batch EURUSD rate view names via resolve_rate_view_names.""" + return resolve_rate_view_names( + conn_or_path, + ["EURUSD"], + [granularity], + require_existing=require_existing, + ) + + class TestResolveRateViewName: """Tests for resolve_rate_view_name and resolve_rate_view_names.""" @@ -90,12 +121,18 @@ class TestResolveRateViewName: "rate_EURUSD__16385", ] - def test_none_path_with_require_existing_raises(self) -> None: + @pytest.mark.parametrize( + "resolve", + [_resolve_rate_view_single, _resolve_rate_view_batch], + ids=["single", "batch"], + ) + def test_none_path_with_require_existing_raises( + self, + resolve: Callable[..., object], + ) -> None: """Test a None path under strict mode raises a clear error.""" with pytest.raises(ValueError, match="SQLite database not found"): - resolve_rate_view_name(None, "EURUSD", "M1", require_existing=True) - with pytest.raises(ValueError, match="SQLite database not found"): - resolve_rate_view_names(None, ["EURUSD"], ["M1"], require_existing=True) + resolve(None, require_existing=True) def test_no_rates_table_falls_back_to_single_timeframe_name( self, @@ -214,12 +251,19 @@ class TestResolveRateViewName: conn.execute('CREATE VIEW "rate_summary" AS SELECT 1 AS close') assert resolve_rate_view_name(db_path, "EURUSD", "M1") == "rate_EURUSD__1" - def test_invalid_granularity_propagates_value_error(self, tmp_path: Path) -> None: + @pytest.mark.parametrize( + "resolve", + [_resolve_rate_view_single, _resolve_rate_view_batch], + ids=["single", "batch"], + ) + def test_invalid_granularity_propagates_value_error( + self, + tmp_path: Path, + resolve: Callable[..., object], + ) -> None: """Test invalid granularities raise ValueError from parse_timeframe.""" with pytest.raises(ValueError, match="Invalid timeframe"): - resolve_rate_view_name(tmp_path / "unused.db", "EURUSD", "BAD") - with pytest.raises(ValueError, match="Invalid timeframe"): - resolve_rate_view_names(tmp_path / "unused.db", ["EURUSD"], ["BAD"]) + resolve(tmp_path / "unused.db", granularity="BAD") def test_resolve_rate_view_names_for_multiple_pairs(self, tmp_path: Path) -> None: """Test batch resolution returns row-major symbol/granularity pairs.""" @@ -292,46 +336,37 @@ class TestResolveRateViewName: create_rate_compatibility_views(conn) assert resolve_rate_view_name(conn, "EURUSD", "M1") == "rate_EURUSD__1" + @pytest.mark.parametrize( + "resolve", + [_resolve_rate_view_single, _resolve_rate_view_batch], + ids=["single", "batch"], + ) def test_require_existing_raises_when_database_missing( self, tmp_path: Path, + resolve: Callable[..., object], ) -> None: """Test strict mode rejects missing database paths.""" db_path = tmp_path / "missing.db" with pytest.raises(ValueError, match="SQLite database not found"): - resolve_rate_view_name( - db_path, - "EURUSD", - "M1", - require_existing=True, - ) - with pytest.raises(ValueError, match="SQLite database not found"): - resolve_rate_view_names( - db_path, - ["EURUSD"], - ["M1"], - require_existing=True, - ) + resolve(db_path, require_existing=True) - def test_require_existing_raises_when_view_missing(self, tmp_path: Path) -> None: + @pytest.mark.parametrize( + "resolve", + [_resolve_rate_view_single, _resolve_rate_view_batch], + ids=["single", "batch"], + ) + def test_require_existing_raises_when_view_missing( + self, + tmp_path: Path, + resolve: Callable[..., object], + ) -> None: """Test strict mode rejects databases without matching rate views.""" db_path = tmp_path / "no-view.db" with sqlite3.connect(db_path) as conn: conn.execute("CREATE TABLE ticks(symbol TEXT, time TEXT)") with pytest.raises(ValueError, match="No rate compatibility view exists"): - resolve_rate_view_name( - db_path, - "EURUSD", - "M1", - require_existing=True, - ) - with pytest.raises(ValueError, match="No rate compatibility view exists"): - resolve_rate_view_names( - db_path, - ["EURUSD"], - ["M1"], - require_existing=True, - ) + resolve(db_path, require_existing=True) def test_require_existing_returns_existing_view(self, tmp_path: Path) -> None: """Test strict mode returns a view when one exists.""" @@ -469,12 +504,24 @@ class TestLoadRateData: frame = load_rate_data_from_connection(conn, table) assert list(frame["close"]) == [1.0] - def test_rejects_missing_database_and_non_file(self, tmp_path: Path) -> None: + @pytest.mark.parametrize( + ("db_path", "match"), + [ + pytest.param( + "missing.db", "SQLite database not found", id="missing-database" + ), + pytest.param(".", "not a file", id="non-file"), + ], + ) + def test_rejects_missing_database_and_non_file( + self, + tmp_path: Path, + db_path: str, + match: str, + ) -> None: """Test path validation for SQLite database inputs.""" - with pytest.raises(ValueError, match="SQLite database not found"): - load_rate_data(tmp_path / "missing.db", "rates") - with pytest.raises(ValueError, match="not a file"): - load_rate_data(tmp_path, "rates") + with pytest.raises(ValueError, match=match): + load_rate_data(tmp_path / db_path, "rates") @pytest.mark.parametrize( ("table", "count", "match"), @@ -547,23 +594,37 @@ class TestLoadRateData: class TestResolveHistorySettings: """Tests for history dataset and timeframe resolution.""" - def test_resolve_history_datasets_defaults_and_empty(self) -> None: - """Test dataset resolution excludes ticks by default.""" - resolved = resolve_history_datasets(None) - assert resolved == set(DEFAULT_HISTORY_DATASETS) - assert Dataset.ticks not in resolved - assert { - Dataset.rates, - Dataset.history_orders, - Dataset.history_deals, - } == resolved - assert resolve_history_datasets(set()) == set() - - def test_resolve_history_datasets_explicit_ticks(self) -> None: - """Test that explicit ticks selection is honored.""" - assert resolve_history_datasets({Dataset.ticks}) == {Dataset.ticks} - all_ds = resolve_history_datasets(set(Dataset)) - assert Dataset.ticks in all_ds + @pytest.mark.parametrize( + ("datasets", "expected"), + [ + ( + None, + { + Dataset.rates, + Dataset.history_orders, + Dataset.history_deals, + }, + ), + ( + cast("set[Dataset]", set()), + cast("set[Dataset]", set()), + ), + ({Dataset.ticks}, {Dataset.ticks}), + (set(Dataset), set(Dataset)), + ], + ids=["defaults", "empty", "explicit-ticks", "all-datasets"], + ) + def test_resolve_history_datasets( + self, + datasets: set[Dataset] | None, + expected: set[Dataset], + ) -> None: + """Test dataset resolution defaults and explicit selections.""" + resolved = resolve_history_datasets(datasets) + assert resolved == expected + if datasets is None: + assert resolved == set(DEFAULT_HISTORY_DATASETS) + assert Dataset.ticks not in resolved def test_resolve_history_timeframes_defaults(self) -> None: """Test default timeframes include all fixed MT5 values.""" @@ -579,15 +640,31 @@ class TestResolveHistorySettings: """Test duplicate aliases for the same timeframe are deduplicated.""" assert resolve_history_timeframes(["M1", "1", "H1"]) == [1, TIMEFRAME_MAP["H1"]] - def test_resolve_history_tick_flags(self) -> None: - """Test tick flag resolution.""" - assert resolve_history_tick_flags("ALL") == -1 - assert resolve_history_tick_flags(2) == 2 + @pytest.mark.parametrize( + ("flags", "expected"), + [("ALL", -1), (2, 2)], + ids=["named-all", "numeric"], + ) + def test_resolve_history_tick_flags( + self, + flags: str | int, + expected: int, + ) -> None: + """Test tick flag resolution accepts named and numeric flags.""" + assert resolve_history_tick_flags(flags) == expected - def test_resolve_granularity_name_falls_back_to_integer(self) -> None: + @pytest.mark.parametrize( + ("timeframe", "expected"), + [(999, "999"), (1, "M1")], + ids=["unknown-integer-fallback", "known-m1"], + ) + def test_resolve_granularity_name_falls_back_to_integer( + self, + timeframe: int, + expected: str, + ) -> None: """Test unknown timeframe constants fall back to integer text.""" - assert resolve_granularity_name(999) == "999" - assert resolve_granularity_name(1) == "M1" + assert resolve_granularity_name(timeframe) == expected def test_resolve_granularity_name_strips_official_prefix( self, @@ -622,19 +699,24 @@ class TestDropFormingRateBar: ) assert df_rate.shape == (3, 2) - def test_returns_empty_frame_when_input_empty(self) -> None: - """Test empty frames stay empty.""" - df_rate = pd.DataFrame(columns=["time", "close"]) - - result = drop_forming_rate_bar(df_rate) - - assert result.empty - assert list(result.columns) == ["time", "close"] - - def test_returns_empty_frame_when_only_forming_bar_present(self) -> None: - """Test a single-bar frame becomes empty after dropping the forming bar.""" - df_rate = pd.DataFrame({"time": [1], "close": [1.1]}) - + @pytest.mark.parametrize( + "df_rate", + [ + pytest.param( + pd.DataFrame(columns=["time", "close"]), + id="empty-input", + ), + pytest.param( + pd.DataFrame({"time": [1], "close": [1.1]}), + id="single-forming-bar", + ), + ], + ) + def test_returns_empty_frame_for_empty_result_cases( + self, + df_rate: pd.DataFrame, + ) -> None: + """Test empty and single-bar frames stay empty after dropping.""" result = drop_forming_rate_bar(df_rate) assert result.empty @@ -644,16 +726,36 @@ class TestDropFormingRateBar: class TestParseSqliteTimestamp: """Tests for parse_sqlite_timestamp.""" - def test_parses_iso_and_pandas_strings(self) -> None: + @pytest.mark.parametrize( + ("value", "expected"), + [ + pytest.param(None, None, id="none"), + pytest.param( + 1_704_067_200, + datetime(2024, 1, 1, tzinfo=UTC), + id="mt5-epoch-seconds", + ), + pytest.param( + datetime.fromisoformat("2024-01-01T00:00:00"), + datetime(2024, 1, 1, tzinfo=UTC), + id="naive-datetime", + ), + pytest.param( + "Jan 1 2024", + datetime(2024, 1, 1, tzinfo=UTC), + id="pandas-string", + ), + pytest.param("not-a-datetime", None, id="invalid-string"), + pytest.param(object(), None, id="unsupported-object"), + ], + ) + def test_parses_various_inputs( + self, + value: object, + expected: datetime | None, + ) -> None: """Test ISO, pandas-compatible, numeric, and datetime values.""" - assert parse_sqlite_timestamp(None) is None - assert parse_sqlite_timestamp(1_704_067_200) == datetime(2024, 1, 1, tzinfo=UTC) - assert parse_sqlite_timestamp( - datetime.fromisoformat("2024-01-01T00:00:00"), - ) == datetime(2024, 1, 1, tzinfo=UTC) - assert parse_sqlite_timestamp("Jan 1 2024") == datetime(2024, 1, 1, tzinfo=UTC) - assert parse_sqlite_timestamp("not-a-datetime") is None - assert parse_sqlite_timestamp(object()) is None + assert parse_sqlite_timestamp(value) == expected class TestIncrementalStart: @@ -770,25 +872,73 @@ class TestIncrementalStart: assert "timeframe" in message assert "time" in message + @pytest.mark.parametrize( + ( + "dataset", + "ddl", + "insert_sql", + "insert_args", + "symbols", + "timeframes", + "start_key", + "db_name", + ), + [ + pytest.param( + Dataset.ticks, + "CREATE TABLE ticks(symbol TEXT, time TEXT)", + "INSERT INTO ticks(symbol, time) VALUES (?, ?)", + ("EURUSD", "not-a-datetime"), + ["EURUSD"], + None, + ("EURUSD", None), + "bad-max-time", + id="ticks-unparseable-max-time", + ), + pytest.param( + Dataset.rates, + ( + "CREATE TABLE rates(" + " symbol TEXT, timeframe INTEGER, time TEXT, open REAL)" + ), + ( + "INSERT INTO rates(symbol, timeframe, time, open)" + " VALUES (?, ?, ?, ?)" + ), + ("EURUSD", 1, "not-a-datetime", 1.0), + ["EURUSD"], + [1], + ("EURUSD", 1), + "bad-rates-max-time", + id="rates-unparseable-max-time", + ), + ], + ) def test_load_incremental_start_skips_unparseable_max_time( self, tmp_path: Path, + dataset: Dataset, + ddl: str, + insert_sql: str, + insert_args: tuple[object, ...], + symbols: list[str], + timeframes: list[int] | None, + start_key: tuple[str, int | None], + db_name: str, ) -> None: """Test grouped resume ignores rows whose MAX(time) cannot be parsed.""" fallback = datetime(2024, 1, 1, tzinfo=UTC) - with sqlite3.connect(tmp_path / "bad-max-time.db") as conn: - conn.execute("CREATE TABLE ticks(symbol TEXT, time TEXT)") - conn.execute( - "INSERT INTO ticks(symbol, time) VALUES (?, ?)", - ("EURUSD", "not-a-datetime"), - ) + with sqlite3.connect(tmp_path / f"{db_name}.db") as conn: + conn.execute(ddl) + conn.execute(insert_sql, insert_args) starts = load_incremental_start_datetimes( conn, - Dataset.ticks, - symbols=["EURUSD"], + dataset, + symbols=symbols, + timeframes=timeframes, fallback_start=fallback, ) - assert starts["EURUSD", None] == fallback + assert starts[start_key] == fallback def test_load_incremental_start_uses_table_max_without_symbol_column( self, @@ -812,30 +962,6 @@ class TestIncrementalStart: assert starts["EURUSD", None] == expected assert starts["GBPUSD", None] == expected - def test_load_incremental_start_skips_unparseable_rates_max_time( - self, - tmp_path: Path, - ) -> None: - """Test grouped rates resume ignores unparseable MAX(time) values.""" - fallback = datetime(2024, 1, 1, tzinfo=UTC) - with sqlite3.connect(tmp_path / "bad-rates-max-time.db") as conn: - conn.execute( - "CREATE TABLE rates(" - " symbol TEXT, timeframe INTEGER, time TEXT, open REAL)", - ) - conn.execute( - "INSERT INTO rates(symbol, timeframe, time, open) VALUES (?, ?, ?, ?)", - ("EURUSD", 1, "not-a-datetime", 1.0), - ) - starts = load_incremental_start_datetimes( - conn, - Dataset.rates, - symbols=["EURUSD"], - timeframes=[1], - fallback_start=fallback, - ) - assert starts["EURUSD", 1] == fallback - class TestDeduplication: """Tests for SQLite deduplication helpers.""" @@ -863,13 +989,24 @@ class TestDeduplication: assert conn.execute("SELECT COUNT(*) FROM rates").fetchone() == (1,) assert conn.execute("SELECT open FROM rates").fetchone() == (9.9,) - def test_drop_duplicates_rejects_invalid_identifiers(self) -> None: + @pytest.mark.parametrize( + ("table", "columns", "match"), + [ + ("bad table", ["id"], "Invalid table name"), + ("rates", ["bad column"], "Invalid column names"), + ], + ids=["invalid-table", "invalid-columns"], + ) + def test_drop_duplicates_rejects_invalid_identifiers( + self, + table: str, + columns: list[str], + match: str, + ) -> None: """Test invalid table or column names raise ValueError.""" cursor = sqlite3.connect(":memory:").cursor() - with pytest.raises(ValueError, match="Invalid table name"): - drop_duplicates_in_table(cursor, "bad table", ["id"]) - with pytest.raises(ValueError, match="Invalid column names"): - drop_duplicates_in_table(cursor, "rates", ["bad column"]) + with pytest.raises(ValueError, match=match): + drop_duplicates_in_table(cursor, table, columns) @pytest.mark.parametrize( ("dataset", "table_sql", "insert_sql", "rows", "columns"), @@ -1247,66 +1384,63 @@ class TestFilterTradeHistoryFrame: class TestIncrementalHistoryDealsHelpers: """Tests for incremental history_deals helper functions.""" - def test_account_event_start_uses_type_column(self, tmp_path: Path) -> None: - """Test account-event start uses non-trade deal types when available.""" - fallback = datetime(2024, 1, 1, tzinfo=UTC) - with sqlite3.connect(tmp_path / "account-start-type.db") as conn: - conn.execute( - "CREATE TABLE history_deals(" - " ticket INTEGER, symbol TEXT, time TEXT, type INTEGER)", - ) - conn.executemany( - "INSERT INTO history_deals(ticket, symbol, time, type)" - " VALUES (?, ?, ?, ?)", + @pytest.mark.parametrize( + ("ddl", "insert_sql", "rows", "expected"), + [ + pytest.param( + ( + "CREATE TABLE history_deals(" + " ticket INTEGER, symbol TEXT, time TEXT, type INTEGER)" + ), + ( + "INSERT INTO history_deals(ticket, symbol, time, type)" + " VALUES (?, ?, ?, ?)" + ), [ (1, "EURUSD", "2024-01-05T00:00:00+00:00", 0), (2, "", "2024-01-08T00:00:00+00:00", 2), ], - ) - assert get_history_deals_account_event_start_datetime( - conn, - fallback_start=fallback, - ) == datetime(2024, 1, 8, tzinfo=UTC) - - def test_account_event_start_falls_back_to_empty_symbol( - self, tmp_path: Path - ) -> None: - """Test account-event start uses empty symbols when type is missing.""" - fallback = datetime(2024, 1, 1, tzinfo=UTC) - with sqlite3.connect(tmp_path / "account-start-symbol.db") as conn: - conn.execute( - "CREATE TABLE history_deals(ticket INTEGER, symbol TEXT, time TEXT)", - ) - conn.executemany( + datetime(2024, 1, 8, tzinfo=UTC), + id="uses-type-column", + ), + pytest.param( + ("CREATE TABLE history_deals( ticket INTEGER, symbol TEXT, time TEXT)"), "INSERT INTO history_deals(ticket, symbol, time) VALUES (?, ?, ?)", [ (1, "EURUSD", "2024-01-05T00:00:00+00:00"), (2, "", "2024-01-07T00:00:00+00:00"), ], - ) - assert get_history_deals_account_event_start_datetime( - conn, - fallback_start=fallback, - ) == datetime(2024, 1, 7, tzinfo=UTC) - - def test_account_event_start_without_identifying_columns( + datetime(2024, 1, 7, tzinfo=UTC), + id="falls-back-to-empty-symbol", + ), + pytest.param( + "CREATE TABLE history_deals(ticket INTEGER, time TEXT)", + "INSERT INTO history_deals(ticket, time) VALUES (?, ?)", + [(1, "2024-01-05T00:00:00+00:00")], + datetime(2024, 1, 1, tzinfo=UTC), + id="without-identifying-columns", + ), + ], + ) + def test_get_history_deals_account_event_start_datetime( self, tmp_path: Path, + ddl: str, + insert_sql: str, + rows: list[tuple[object, ...]], + expected: datetime, ) -> None: - """Test account-event start falls back when type and symbol are missing.""" + """Test account-event start resolution across identifying-column variants.""" fallback = datetime(2024, 1, 1, tzinfo=UTC) - with sqlite3.connect(tmp_path / "account-start-fallback.db") as conn: - conn.execute("CREATE TABLE history_deals(ticket INTEGER, time TEXT)") - conn.execute( - "INSERT INTO history_deals(ticket, time) VALUES (?, ?)", - (1, "2024-01-05T00:00:00+00:00"), - ) + with sqlite3.connect(tmp_path / "account-start.db") as conn: + conn.execute(ddl) + conn.executemany(insert_sql, rows) assert ( get_history_deals_account_event_start_datetime( conn, fallback_start=fallback, ) - == fallback + == expected ) def test_filter_incremental_history_deals_frame(self) -> None: @@ -1335,86 +1469,90 @@ class TestIncrementalHistoryDealsHelpers: ) assert filtered["ticket"].tolist() == [2, 3, 5] - def test_filter_incremental_excludes_symbolized_account_events_from_trade_cursor( + @pytest.mark.parametrize( + ("frame", "start_by_symbol", "account_event_start", "expected_tickets"), + [ + pytest.param( + pd.DataFrame({ + "ticket": [1], + "symbol": ["EURUSD"], + "time": [datetime(2024, 1, 5, tzinfo=UTC).isoformat()], + "type": [2], + }), + {"EURUSD": datetime(2024, 1, 1, tzinfo=UTC)}, + datetime(2024, 1, 10, tzinfo=UTC), + [], + id="excludes-symbolized-account-events-from-trade-cursor", + ), + pytest.param( + pd.DataFrame({ + "ticket": [1], + "time": ["2024-01-03T00:00:00+00:00"], + "type": [2], + }), + {"EURUSD": datetime(2024, 1, 10, tzinfo=UTC)}, + datetime(2024, 1, 2, tzinfo=UTC), + [1], + id="keeps-account-events-without-symbol-column", + ), + ], + ) + def test_filter_incremental_account_event_edge_cases( self, + frame: pd.DataFrame, + start_by_symbol: dict[str, datetime], + account_event_start: datetime, + expected_tickets: list[int], ) -> None: - """Test symbolized account events follow only account_event_start. + """Test account-event rows are scoped by symbol column presence. - An account-event row with a matching symbol must not be kept by the - EURUSD trade cursor when its time is after the trade start but before + A symbolized account-event row follows only the EURUSD trade cursor + and is excluded when its time falls before account_event_start; when + history_deals has no symbol column, account events are kept using account_event_start. """ - trade_cursor = datetime(2024, 1, 1, tzinfo=UTC) - account_event_start = datetime(2024, 1, 10, tzinfo=UTC) - row_time = datetime(2024, 1, 5, tzinfo=UTC) - frame = pd.DataFrame({ - "ticket": [1], - "symbol": ["EURUSD"], - "time": [row_time.isoformat()], - "type": [2], - }) filtered = filter_incremental_history_deals_frame( frame, ["EURUSD"], - {"EURUSD": trade_cursor}, + start_by_symbol, account_event_start, ) - assert filtered.empty + assert filtered["ticket"].tolist() == expected_tickets - def test_filter_incremental_rejects_rows_without_parseable_time(self) -> None: - """Test incremental filtering drops rows when time cannot be parsed.""" - frame = pd.DataFrame({ - "ticket": [1], - "symbol": ["EURUSD"], - "time": ["not-a-datetime"], - "type": [0], - }) - filtered = filter_incremental_history_deals_frame( - frame, - ["EURUSD"], - {"EURUSD": datetime(2024, 1, 1, tzinfo=UTC)}, - datetime(2024, 1, 1, tzinfo=UTC), - ) - assert filtered.empty - - def test_filter_incremental_keeps_account_events_without_symbol_column( + @pytest.mark.parametrize( + "frame", + [ + pytest.param( + pd.DataFrame({ + "ticket": [1], + "symbol": ["EURUSD"], + "time": ["not-a-datetime"], + "type": [0], + }), + id="unparseable-time", + ), + pytest.param( + pd.DataFrame({ + "ticket": [1], + "time": ["2024-01-03T00:00:00+00:00"], + }), + id="trade-rows-without-symbol-column", + ), + pytest.param( + pd.DataFrame({ + "ticket": [1], + "symbol": ["EURUSD"], + "type": [0], + }), + id="rows-without-time-column", + ), + ], + ) + def test_filter_incremental_returns_empty_for_invalid_rows( self, + frame: pd.DataFrame, ) -> None: - """Test account events are kept when history_deals has no symbol column.""" - frame = pd.DataFrame({ - "ticket": [1], - "time": ["2024-01-03T00:00:00+00:00"], - "type": [2], - }) - filtered = filter_incremental_history_deals_frame( - frame, - ["EURUSD"], - {"EURUSD": datetime(2024, 1, 10, tzinfo=UTC)}, - datetime(2024, 1, 2, tzinfo=UTC), - ) - assert filtered["ticket"].tolist() == [1] - - def test_filter_incremental_skips_trade_rows_without_symbol_column(self) -> None: - """Test trade rows are excluded when symbol column is unavailable.""" - frame = pd.DataFrame({ - "ticket": [1], - "time": ["2024-01-03T00:00:00+00:00"], - }) - filtered = filter_incremental_history_deals_frame( - frame, - ["EURUSD"], - {"EURUSD": datetime(2024, 1, 1, tzinfo=UTC)}, - datetime(2024, 1, 1, tzinfo=UTC), - ) - assert filtered.empty - - def test_filter_incremental_rejects_rows_without_time_column(self) -> None: - """Test incremental filtering drops rows when time column is missing.""" - frame = pd.DataFrame({ - "ticket": [1], - "symbol": ["EURUSD"], - "type": [0], - }) + """Test incremental filtering drops rows that cannot be evaluated.""" filtered = filter_incremental_history_deals_frame( frame, ["EURUSD"], @@ -1932,56 +2070,50 @@ class TestIncrementalHistoryDeals: assert rows == [(1, "EURUSD", 0), (2, "GBPUSD", 0), (4, "", 2)] assert row_count == (3,) - def test_records_account_event_dedup_scope_without_type_column( - self, - tmp_path: Path, - ) -> None: - """Test account-event dedup scope falls back to empty symbols.""" - client = MagicMock() - client.history_deals_get_as_df.return_value = pd.DataFrame({ - "ticket": [1], - "symbol": [""], - "time": ["2024-01-02T00:00:00+00:00"], - }) - start = datetime(2024, 1, 1, tzinfo=UTC) - end = datetime(2024, 1, 3, tzinfo=UTC) - with sqlite3.connect(tmp_path / "deals-without-type.db") as conn: - conn.execute( + @pytest.mark.parametrize( + ("frame", "ddl", "db_name"), + [ + pytest.param( + pd.DataFrame({ + "ticket": [1], + "symbol": [""], + "time": ["2024-01-02T00:00:00+00:00"], + }), "CREATE TABLE history_deals( ticket INTEGER, symbol TEXT, time TEXT)", - ) - write_incremental_datasets( - conn, - client, - ["EURUSD"], - {Dataset.history_deals}, - [], - 0, - start, - end, - deduplicate=True, - create_rate_views=False, - with_views=False, - include_account_events=True, - ) - assert conn.execute("SELECT COUNT(*) FROM history_deals").fetchone() == (1,) - - def test_skips_symbol_dedup_scope_when_symbol_column_missing( + "deals-without-type.db", + id="dedup-scope-without-type-column", + ), + pytest.param( + pd.DataFrame({ + "ticket": [1], + "time": ["2024-01-02T00:00:00+00:00"], + "type": [2], + }), + "CREATE TABLE history_deals(ticket INTEGER, time TEXT, type INTEGER)", + "no-symbol-deals.db", + id="dedup-scope-without-symbol-column", + ), + ], + ) + def test_account_event_dedup_scope_with_missing_column( self, tmp_path: Path, + frame: pd.DataFrame, + ddl: str, + db_name: str, ) -> None: - """Test account-event writes skip per-symbol dedup without symbol column.""" + """Test account-event dedup falls back safely when a scope column is missing. + + Without a type column, dedup scope falls back to empty symbols; without + a symbol column, per-symbol dedup scope is skipped. Both still write + the row. + """ client = MagicMock() - client.history_deals_get_as_df.return_value = pd.DataFrame({ - "ticket": [1], - "time": ["2024-01-02T00:00:00+00:00"], - "type": [2], - }) + client.history_deals_get_as_df.return_value = frame start = datetime(2024, 1, 1, tzinfo=UTC) end = datetime(2024, 1, 3, tzinfo=UTC) - with sqlite3.connect(tmp_path / "no-symbol-deals.db") as conn: - conn.execute( - "CREATE TABLE history_deals(ticket INTEGER, time TEXT, type INTEGER)", - ) + with sqlite3.connect(tmp_path / db_name) as conn: + conn.execute(ddl) write_incremental_datasets( conn, client, @@ -2196,22 +2328,69 @@ class TestRateSourceHelpers: targets = build_rate_targets([], ["M1", "H1"], allow_missing_symbol=True) assert resolve_rate_tables(None, targets, ["t1", "t2"]) == ["t1", "t2"] - def test_resolve_rate_tables_rejects_mismatched_explicit_count(self) -> None: - """Test explicit table count must match the number of targets.""" - targets = build_rate_targets(["EURUSD"], ["M1"]) - with pytest.raises(ValueError, match="Expected 1 explicit table"): - resolve_rate_tables(None, targets, ["t1", "t2"]) - - def test_resolve_rate_tables_rejects_empty_targets(self) -> None: - """Test resolving requires at least one target.""" - with pytest.raises(ValueError, match="At least one rate target"): - resolve_rate_tables(None, []) - - def test_resolve_rate_tables_requires_symbol_without_explicit(self) -> None: - """Test None-symbol targets require explicit tables.""" - targets = build_rate_targets([], ["M1"], allow_missing_symbol=True) - with pytest.raises(ValueError, match="without a symbol"): - resolve_rate_tables(None, targets) + @pytest.mark.parametrize( + ("targets", "explicit_tables", "path_kind", "require_existing", "match"), + [ + pytest.param( + build_rate_targets(["EURUSD"], ["M1"]), + ["t1", "t2"], + "none", + False, + "Expected 1 explicit table", + id="mismatched-explicit-count", + ), + pytest.param( + [], + None, + "none", + False, + "At least one rate target", + id="empty-targets", + ), + pytest.param( + build_rate_targets([], ["M1"], allow_missing_symbol=True), + None, + "none", + False, + "without a symbol", + id="none-symbol-without-explicit", + ), + pytest.param( + build_rate_targets(["EURUSD"], ["M1"]), + None, + "none", + True, + "SQLite database not found", + id="none-path-require-existing", + ), + pytest.param( + build_rate_targets(["EURUSD"], ["M1"]), + None, + "missing", + True, + "SQLite database not found", + id="missing-db-require-existing", + ), + ], + ) + def test_resolve_rate_tables_rejects_invalid_inputs( + self, + tmp_path: Path, + targets: list[RateTarget], + explicit_tables: list[str] | None, + path_kind: str, + require_existing: bool, + match: str, + ) -> None: + """Test resolve_rate_tables input and strict-mode validation.""" + db_path = None if path_kind == "none" else tmp_path / "missing.db" + with pytest.raises(ValueError, match=match): + resolve_rate_tables( + db_path, + targets, + explicit_tables=explicit_tables, + require_existing=require_existing, + ) def test_resolve_rate_tables_resolves_view_names(self) -> None: """Test symbol targets resolve to default view names without a database.""" @@ -2221,22 +2400,6 @@ class TestRateSourceHelpers: "rate_EURUSD__16385", ] - def test_resolve_rate_tables_none_path_with_require_existing_raises(self) -> None: - """Test strict mode rejects a missing database path.""" - targets = build_rate_targets(["EURUSD"], ["M1"]) - with pytest.raises(ValueError, match="SQLite database not found"): - resolve_rate_tables(None, targets, require_existing=True) - - def test_resolve_rate_tables_missing_db_with_require_existing_raises( - self, - tmp_path: Path, - ) -> None: - """Test strict mode rejects a non-existing database path.""" - db_path = tmp_path / "missing.db" - targets = build_rate_targets(["EURUSD"], ["M1"]) - with pytest.raises(ValueError, match="SQLite database not found"): - resolve_rate_tables(db_path, targets, require_existing=True) - def test_resolve_rate_tables_missing_view_with_require_existing_raises( self, tmp_path: Path, @@ -2428,22 +2591,61 @@ class TestRateSourceHelpers: ) assert set(result) == {(None, 1)} - def test_load_rate_series_rejects_non_positive_count(self) -> None: - """Test loading requires a positive count.""" - targets = build_rate_targets(["EURUSD"], ["M1"]) - with pytest.raises(ValueError, match="count must be positive"): - load_rate_series_from_sqlite("unused.db", targets, count=0) - - def test_load_rate_series_rejects_empty_targets(self) -> None: - """Test loading requires at least one target before opening SQLite.""" - with pytest.raises(ValueError, match="At least one rate target"): - load_rate_series_from_sqlite("unused.db", [], count=1) - - def test_load_rate_series_requires_symbol_without_explicit_tables(self) -> None: - """Test None-symbol targets require explicit tables before opening SQLite.""" - targets = build_rate_targets([], ["M1"], allow_missing_symbol=True) - with pytest.raises(ValueError, match="without a symbol"): - load_rate_series_from_sqlite("unused.db", targets, count=1) + @pytest.mark.parametrize( + ("targets", "count", "explicit_tables", "match"), + [ + pytest.param( + build_rate_targets(["EURUSD"], ["M1"]), + 0, + None, + "count must be positive", + id="non-positive-count", + ), + pytest.param( + [], + 1, + None, + "At least one rate target", + id="empty-targets", + ), + pytest.param( + build_rate_targets([], ["M1"], allow_missing_symbol=True), + 1, + None, + "without a symbol", + id="none-symbol-without-explicit", + ), + pytest.param( + [RateTarget("EURUSD", 1), RateTarget("EURUSD", "M1")], + 1, + None, + r"Duplicate rate target: \('EURUSD', 1\)", + id="duplicate-targets", + ), + pytest.param( + [RateTarget("EURUSD", 1), RateTarget("EURUSD", 1)], + 1, + ["custom_view", "custom_view"], + r"Duplicate rate target: \('EURUSD', 1\)", + id="duplicate-targets-with-explicit", + ), + ], + ) + def test_load_rate_series_rejects_invalid_inputs( + self, + targets: list[RateTarget], + count: int, + explicit_tables: list[str] | None, + match: str, + ) -> None: + """Test load_rate_series_from_sqlite validation runs before opening SQLite.""" + with pytest.raises(ValueError, match=match): + load_rate_series_from_sqlite( + "unused.db", + targets, + count=count, + explicit_tables=explicit_tables, + ) def test_load_rate_series_requires_existing_managed_views( self, @@ -2463,36 +2665,3 @@ class TestRateSourceHelpers: targets = build_rate_targets(["EURUSD"], ["M1"]) with pytest.raises(ValueError, match="No rate compatibility view exists"): load_rate_series_from_sqlite(db_path, targets, count=1) - - def test_load_rate_series_rejects_duplicate_targets(self) -> None: - """Test duplicate (symbol, timeframe) targets are rejected.""" - targets = [ - RateTarget("EURUSD", 1), - RateTarget("EURUSD", "M1"), - ] - with pytest.raises(ValueError, match=r"Duplicate rate target: \('EURUSD', 1\)"): - load_rate_series_from_sqlite("unused.db", targets, count=1) - - def test_load_rate_series_rejects_duplicate_targets_with_explicit_tables( - self, - tmp_path: Path, - ) -> None: - """Test duplicate targets are rejected even with explicit tables.""" - db_path = tmp_path / "duplicate-explicit.db" - with sqlite3.connect(db_path) as conn: - conn.execute("CREATE TABLE custom_view(time TEXT, close REAL)") - conn.execute( - "INSERT INTO custom_view(time, close) VALUES (?, ?)", - ("2024-01-01T00:00:00+00:00", 1.0), - ) - targets = [ - RateTarget("EURUSD", 1), - RateTarget("EURUSD", 1), - ] - with pytest.raises(ValueError, match=r"Duplicate rate target: \('EURUSD', 1\)"): - load_rate_series_from_sqlite( - db_path, - targets, - count=1, - explicit_tables=["custom_view", "custom_view"], - ) diff --git a/tests/test_sdk.py b/tests/test_sdk.py index 71b394f..1a7793d 100644 --- a/tests/test_sdk.py +++ b/tests/test_sdk.py @@ -14,6 +14,7 @@ from pdmt5 import Mt5RuntimeError, Mt5TradingError from pytest_mock import MockerFixture # noqa: TC002 if TYPE_CHECKING: + from collections.abc import Callable from pathlib import Path from pdmt5 import Mt5Config, Mt5DataClient @@ -65,7 +66,7 @@ from mt5cli.sdk import ( update_observability_with_config, version, ) -from mt5cli.utils import Dataset, IfExists, coerce_login +from mt5cli.utils import Dataset, IfExists, coerce_login, parse_timeframe class _TerminalInfo(NamedTuple): @@ -171,6 +172,10 @@ def _build_history_client(mocker: MockerFixture) -> MagicMock: return client +def _mt5_cli_client_with_injected_client(connected: MagicMock) -> Mt5CliClient: + return Mt5CliClient(client=connected) + + class TestConnectionLifecycle: """Tests for MT5 connection lifecycle helpers.""" @@ -254,12 +259,34 @@ class TestConnectionLifecycle: client = Mt5CliClient() client.__exit__(None, None, None) - def test_injected_client_is_reused_and_not_shutdown(self) -> None: - """Test injected connected clients are not initialized or shut down.""" + @pytest.mark.parametrize( + "make_client", + [ + pytest.param( + Mt5CliClient.from_connected_client, + id="from-connected-client-classmethod", + ), + pytest.param( + _mt5_cli_client_with_injected_client, + id="constructor-client-kwarg", + ), + ], + ) + def test_injected_client_is_reused_and_not_shutdown( + self, + make_client: Callable[[MagicMock], Mt5CliClient], + ) -> None: + """Test injected connected clients are not initialized or shut down. + + Both the from_connected_client classmethod and the constructor's + client kwarg must produce the same non-owning lifecycle: no login or + shutdown of the injected client, and continued usability after the + context manager exits. + """ connected = MagicMock() connected.account_info_as_df.return_value = pd.DataFrame({"a": [1]}) connected.terminal_info_as_df.return_value = pd.DataFrame({"b": [2]}) - with Mt5CliClient.from_connected_client(connected) as client: + with make_client(connected) as client: result = client.account_info() assert result.to_dict("list") == {"a": [1]} connected.initialize_and_login_mt5.assert_not_called() @@ -269,17 +296,6 @@ class TestConnectionLifecycle: assert after_exit.to_dict("list") == {"b": [2]} connected.terminal_info_as_df.assert_called_once() - def test_constructor_injected_client_is_reused_and_not_shutdown(self) -> None: - """Test constructor injection has the same non-owning lifecycle.""" - connected = MagicMock() - connected.terminal_info_as_df.return_value = pd.DataFrame({"b": [2]}) - client = Mt5CliClient(client=connected) - with client: - result = client.terminal_info() - assert result.to_dict("list") == {"b": [2]} - connected.initialize_and_login_mt5.assert_not_called() - connected.shutdown.assert_not_called() - class TestModuleFunctions: """Tests for module-level SDK wrappers.""" @@ -339,55 +355,81 @@ class TestModuleFunctions: class TestMt5CliClient: """Tests for Mt5CliClient SDK methods.""" - def test_copy_rates_range_returns_dataframe( + @pytest.mark.parametrize( + ("call", "expected_method", "expected_kwargs"), + [ + pytest.param( + lambda: Mt5CliClient().copy_rates_range( + "EURUSD", + "D1", + "2024-01-01", + "2024-02-01", + ), + "copy_rates_range_as_df", + { + "symbol": "EURUSD", + "timeframe": 16408, + "date_from": datetime(2024, 1, 1, tzinfo=UTC), + "date_to": datetime(2024, 2, 1, tzinfo=UTC), + }, + id="copy_rates_range-normalizes-dates-and-timeframe", + ), + pytest.param( + lambda: Mt5CliClient().copy_ticks_from( + "EURUSD", + "2024-01-01", + 100, + "INFO", + ), + "copy_ticks_from_as_df", + { + "symbol": "EURUSD", + "date_from": datetime(2024, 1, 1, tzinfo=UTC), + "count": 100, + "flags": 1, + }, + id="copy_ticks_from-parses-string-flags", + ), + pytest.param( + lambda: Mt5CliClient().history_orders( + date_from="2024-01-01", + date_to="2024-02-01", + ), + "history_orders_get_as_df", + { + "date_from": datetime(2024, 1, 1, tzinfo=UTC), + "date_to": datetime(2024, 2, 1, tzinfo=UTC), + "group": None, + "symbol": None, + "ticket": None, + "position": None, + }, + id="history_orders-parses-string-dates", + ), + pytest.param( + lambda: Mt5CliClient().latest_rates("EURUSD", "M1", 5, start_pos=2), + "copy_rates_from_pos_as_df", + { + "symbol": "EURUSD", + "timeframe": 1, + "start_pos": 2, + "count": 5, + }, + id="latest_rates-wraps-copy_rates_from_pos", + ), + ], + ) + def test_method_delegates_with_normalization( self, mock_client: MagicMock, + call: Callable[[], object], + expected_method: str, + expected_kwargs: dict[str, object], ) -> None: - """Test that copy_rates_range returns a DataFrame.""" - df = Mt5CliClient().copy_rates_range( - "EURUSD", - "D1", - "2024-01-01", - "2024-02-01", - ) - assert isinstance(df, pd.DataFrame) - mock_client.copy_rates_range_as_df.assert_called_once_with( - symbol="EURUSD", - timeframe=16408, - date_from=datetime(2024, 1, 1, tzinfo=UTC), - date_to=datetime(2024, 2, 1, tzinfo=UTC), - ) - - def test_copy_ticks_from_parses_flags( - self, - mock_client: MagicMock, - ) -> None: - """Test that string tick flags are parsed.""" - Mt5CliClient().copy_ticks_from("EURUSD", "2024-01-01", 100, "INFO") - mock_client.copy_ticks_from_as_df.assert_called_once_with( - symbol="EURUSD", - date_from=datetime(2024, 1, 1, tzinfo=UTC), - count=100, - flags=1, - ) - - def test_history_orders_accepts_string_dates( - self, - mock_client: MagicMock, - ) -> None: - """Test that string datetime inputs are parsed.""" - Mt5CliClient().history_orders( - date_from="2024-01-01", - date_to="2024-02-01", - ) - mock_client.history_orders_get_as_df.assert_called_once_with( - date_from=datetime(2024, 1, 1, tzinfo=UTC), - date_to=datetime(2024, 2, 1, tzinfo=UTC), - group=None, - symbol=None, - ticket=None, - position=None, - ) + """Mt5CliClient methods normalize inputs and forward them on.""" + result = call() + assert isinstance(result, pd.DataFrame) + getattr(mock_client, expected_method).assert_called_once_with(**expected_kwargs) def test_module_function_delegates_to_client( self, @@ -403,19 +445,6 @@ class TestMt5CliClient: assert isinstance(df, pd.DataFrame) mock_client.copy_rates_range_as_df.assert_called_once() - def test_latest_rates_delegates_to_copy_rates_from_pos( - self, - mock_client: MagicMock, - ) -> None: - """Test latest_rates is a convenience wrapper for positional rates.""" - Mt5CliClient().latest_rates("EURUSD", "M1", 5, start_pos=2) - mock_client.copy_rates_from_pos_as_df.assert_called_once_with( - symbol="EURUSD", - timeframe=1, - start_pos=2, - count=5, - ) - def test_latest_rates_rejects_non_positive_count(self) -> None: """Test latest_rates validates count.""" with pytest.raises(ValueError, match="count must be positive"): @@ -516,47 +545,58 @@ class TestMt5CliClient: with pytest.raises(ValueError, match="hours must be positive"): Mt5CliClient().recent_history_deals(0) - def test_mt5_summary_returns_status_mapping( + @pytest.mark.parametrize( + ("terminal_info_value", "account_info_value", "expected"), + [ + ( + {"connected": True}, + {"login": 123}, + { + "version": [5, 0, 1], + "terminal_info": {"connected": True}, + "account_info": {"login": 123}, + "symbols_total": 42, + }, + ), + ( + _TerminalInfo( + connected=True, + path="terminal.exe", + ), + _AccountInfo( + login=123, + limits={"modes": ("netting", "hedging"), "servers": ["demo"]}, + ), + { + "version": [5, 0, 1], + "terminal_info": {"connected": True, "path": "terminal.exe"}, + "account_info": { + "login": 123, + "limits": { + "modes": ["netting", "hedging"], + "servers": ["demo"], + }, + }, + "symbols_total": 42, + }, + ), + ], + ids=["raw-mappings", "namedtuple-normalization"], + ) + def test_mt5_summary_success_cases( self, mock_client: MagicMock, + terminal_info_value: object, + account_info_value: object, + expected: dict[str, object], ) -> None: - """Test mt5_summary calls raw terminal/account status methods.""" + """Test mt5_summary returns normalized plain-Python status mappings.""" mock_client.version.return_value = (5, 0, 1) - mock_client.terminal_info.return_value = {"connected": True} - mock_client.account_info.return_value = {"login": 123} - mock_client.symbols_total.return_value = 42 - assert mt5_summary() == { - "version": [5, 0, 1], - "terminal_info": {"connected": True}, - "account_info": {"login": 123}, - "symbols_total": 42, - } - - def test_mt5_summary_normalizes_namedtuple_values( - self, - mock_client: MagicMock, - ) -> None: - """Test mt5_summary returns structured plain Python values.""" - mock_client.version.return_value = (5, 0, 1) - mock_client.terminal_info.return_value = _TerminalInfo( - connected=True, - path="terminal.exe", - ) - mock_client.account_info.return_value = _AccountInfo( - login=123, - limits={"modes": ("netting", "hedging"), "servers": ["demo"]}, - ) + mock_client.terminal_info.return_value = terminal_info_value + mock_client.account_info.return_value = account_info_value mock_client.symbols_total.return_value = 42 - assert mt5_summary() == { - "version": [5, 0, 1], - "terminal_info": {"connected": True, "path": "terminal.exe"}, - "account_info": { - "login": 123, - "limits": {"modes": ["netting", "hedging"], "servers": ["demo"]}, - }, - "symbols_total": 42, - } + assert mt5_summary() == expected def test_mt5_summary_as_df_stringifies_nested_values( self, @@ -587,28 +627,32 @@ class TestMt5CliClient: "symbols_total": 42, } - def test_mt5_summary_missing_method_raises_clear_error(self) -> None: - """Test mt5_summary fails clearly when a required method is missing.""" - client = Mt5CliClient( - client=cast("Mt5DataClient", _MissingSummaryMethodClient()), - ) + @pytest.mark.parametrize( + ("client_cls", "exc", "match"), + [ + ( + _MissingSummaryMethodClient, + AttributeError, + "MT5 client is missing required method: account_info", + ), + ( + _NonCallableSummaryMethodClient, + TypeError, + "MT5 client attribute is not callable: version", + ), + ], + ids=["missing-method", "non-callable-method"], + ) + def test_mt5_summary_rejects_bad_client( + self, + client_cls: type[object], + exc: type[BaseException], + match: str, + ) -> None: + """Test mt5_summary fails clearly when a required method is bad.""" + client = Mt5CliClient(client=cast("Mt5DataClient", client_cls())) - with pytest.raises( - AttributeError, - match="MT5 client is missing required method: account_info", - ): - client.mt5_summary() - - def test_mt5_summary_non_callable_method_raises_clear_error(self) -> None: - """Test mt5_summary fails clearly when a required method is not callable.""" - client = Mt5CliClient( - client=cast("Mt5DataClient", _NonCallableSummaryMethodClient()), - ) - - with pytest.raises( - TypeError, - match="MT5 client attribute is not callable: version", - ): + with pytest.raises(exc, match=match): client.mt5_summary() @@ -620,21 +664,62 @@ class TestCollectHistory: """Create a mocked Mt5DataClient with history-style DataFrames.""" return _build_history_client(mocker) - def test_collect_history_writes_default_tables( + @pytest.mark.parametrize( + ( + "datasets", + "expected_rates_calls", + "expected_ticks_calls", + "required_tables", + "forbidden_table", + ), + [ + pytest.param( + None, + 2, + 0, + {"rates", "history_orders", "history_deals"}, + "ticks", + id="default-excludes-ticks", + ), + pytest.param( + {Dataset.ticks}, + 0, + 2, + {"ticks"}, + "rates", + id="explicit-ticks", + ), + ], + ) + def test_collect_history_default_and_ticks_dataset( self, tmp_path: Path, history_client: MagicMock, + datasets: set[Dataset] | None, + expected_rates_calls: int, + expected_ticks_calls: int, + required_tables: set[str], + forbidden_table: str, ) -> None: - """Test that collect_history default excludes ticks.""" + """Test default vs explicit ticks dataset selection for collect_history.""" output = tmp_path / "history.db" - collect_history( - output, - ["EURUSD", "GBPUSD"], - "2024-01-01", - "2024-02-01", - ) - assert history_client.copy_rates_range_as_df.call_count == 2 - assert history_client.copy_ticks_range_as_df.call_count == 0 + if datasets is None: + collect_history( + output, + ["EURUSD", "GBPUSD"], + "2024-01-01", + "2024-02-01", + ) + else: + collect_history( + output, + ["EURUSD", "GBPUSD"], + "2024-01-01", + "2024-02-01", + datasets=datasets, + ) + assert history_client.copy_rates_range_as_df.call_count == expected_rates_calls + assert history_client.copy_ticks_range_as_df.call_count == expected_ticks_calls with sqlite3.connect(output) as conn: tables = { row[0] @@ -642,34 +727,8 @@ class TestCollectHistory: "SELECT name FROM sqlite_master WHERE type='table'", ).fetchall() } - assert {"rates", "history_orders", "history_deals"} <= tables - assert "ticks" not in tables - - def test_collect_history_explicit_ticks_dataset( - self, - tmp_path: Path, - history_client: MagicMock, - ) -> None: - """Test that explicit datasets={Dataset.ticks} writes the ticks table.""" - output = tmp_path / "history.db" - collect_history( - output, - ["EURUSD", "GBPUSD"], - "2024-01-01", - "2024-02-01", - datasets={Dataset.ticks}, - ) - assert history_client.copy_ticks_range_as_df.call_count == 2 - assert history_client.copy_rates_range_as_df.call_count == 0 - with sqlite3.connect(output) as conn: - tables = { - row[0] - for row in conn.execute( - "SELECT name FROM sqlite_master WHERE type='table'", - ).fetchall() - } - assert "ticks" in tables - assert "rates" not in tables + assert required_tables <= tables + assert forbidden_table not in tables def test_collect_history_with_views( self, @@ -832,41 +891,47 @@ class TestUpdateHistory: "SELECT name FROM sqlite_master WHERE name = 'cash_events'", ).fetchone() == ("cash_events",) + @pytest.mark.parametrize( + ("kwargs", "match"), + [ + ({"symbols": []}, "At least one symbol"), + ( + {"symbols": ["EURUSD"], "lookback_hours": 0}, + "lookback_hours must be positive", + ), + ( + { + "symbols": ["EURUSD"], + "datasets": {Dataset.rates}, + "timeframes": ["BAD"], + }, + "Invalid timeframe", + ), + ( + { + "symbols": ["EURUSD"], + "datasets": {Dataset.ticks}, + "flags": "BAD", + }, + "Invalid tick flags", + ), + ], + ids=["empty-symbols", "non-positive-lookback", "bad-timeframe", "bad-flags"], + ) def test_update_history_rejects_invalid_inputs( self, connected_client: MagicMock, tmp_path: Path, + kwargs: dict[str, object], + match: str, ) -> None: """Test validation errors for incremental history updates.""" output = tmp_path / "invalid-update.db" - with pytest.raises(ValueError, match="At least one symbol"): + with pytest.raises(ValueError, match=match): update_history( client=connected_client, output=output, - symbols=[], - ) - with pytest.raises(ValueError, match="lookback_hours must be positive"): - update_history( - client=connected_client, - output=output, - symbols=["EURUSD"], - lookback_hours=0, - ) - with pytest.raises(ValueError, match="Invalid timeframe"): - update_history( - client=connected_client, - output=output, - symbols=["EURUSD"], - datasets={Dataset.rates}, - timeframes=["BAD"], - ) - with pytest.raises(ValueError, match="Invalid tick flags"): - update_history( - client=connected_client, - output=output, - symbols=["EURUSD"], - datasets={Dataset.ticks}, - flags="BAD", + **kwargs, # type: ignore[arg-type] ) def test_update_history_noops_for_empty_datasets( @@ -887,13 +952,23 @@ class TestUpdateHistory: writer.assert_not_called() connect.assert_not_called() - def test_update_history_uses_all_default_timeframes( + @pytest.mark.parametrize( + ("timeframes", "expected"), + [ + (None, [parse_timeframe(t) for t in DEFAULT_HISTORY_TIMEFRAMES]), + (["M1", "H1"], [1, 16385]), + ], + ids=["default", "specified"], + ) + def test_update_history_resolves_timeframes( self, connected_client: MagicMock, mocker: MockerFixture, tmp_path: Path, + timeframes: list[str] | None, + expected: list[int], ) -> None: - """Test that timeframes=None writes rates for all default MT5 timeframes.""" + """Test update_history writes all default or specified rate timeframes.""" timeframes_written: list[int] = [] def capture( @@ -906,42 +981,14 @@ class TestUpdateHistory: mocker.patch("mt5cli.sdk.write_incremental_datasets", side_effect=capture) update_history( client=connected_client, - output=tmp_path / "default-timeframes.db", + output=tmp_path / "timeframes.db", symbols=["EURUSD"], datasets={Dataset.rates}, - timeframes=None, + timeframes=timeframes, lookback_hours=1, date_to=datetime(2024, 1, 1, tzinfo=UTC), ) - assert len(timeframes_written) == len(DEFAULT_HISTORY_TIMEFRAMES) - - def test_update_history_uses_specified_timeframes( - self, - connected_client: MagicMock, - mocker: MockerFixture, - tmp_path: Path, - ) -> None: - """Test explicit timeframes limit rate updates.""" - timeframes_written: list[int] = [] - - def capture( - *args: object, - **_kwargs: object, - ) -> tuple[set[Dataset], dict[Dataset, set[str]]]: - timeframes_written.extend(args[4]) # type: ignore[arg-type] - return set(), {} - - mocker.patch("mt5cli.sdk.write_incremental_datasets", side_effect=capture) - update_history( - client=connected_client, - output=tmp_path / "specific-timeframes.db", - symbols=["EURUSD"], - datasets={Dataset.rates}, - timeframes=["M1", "H1"], - lookback_hours=1, - date_to=datetime(2024, 1, 1, tzinfo=UTC), - ) - assert timeframes_written == [1, 16385] + assert timeframes_written == expected def test_update_history_updates_ticks_and_orders( self, @@ -1601,18 +1648,31 @@ class TestCollectLatestClosedRatesForAccounts: pd.DataFrame({"time": [1, 2], "close": [1.1, 1.2]}), ) - def test_rejects_forming_bar_only_frames(self, mocker: MockerFixture) -> None: - """Test empty results after dropping the forming bar raise ValueError.""" + @pytest.mark.parametrize( + ("rates_frame", "kwargs"), + [ + (pd.DataFrame({"time": [1], "close": [1.1]}), {"count": 1}), + (pd.DataFrame(columns=["time", "close"]), {"count": 1, "start_pos": 1}), + ], + ids=["forming-bar-only", "empty-start-pos-nonzero"], + ) + def test_rejects_empty_effective_frames( + self, + mocker: MockerFixture, + rates_frame: pd.DataFrame, + kwargs: dict[str, object], + ) -> None: + """Test empty effective frames raise after start_pos/forming-bar handling.""" mocker.patch( "mt5cli.sdk.collect_latest_rates_for_accounts_with_retries", - return_value={("EURUSD", 1): pd.DataFrame({"time": [1], "close": [1.1]})}, + return_value={("EURUSD", 1): rates_frame}, ) with pytest.raises(ValueError, match="Rate data is empty"): collect_latest_closed_rates_for_accounts( [AccountSpec(symbols=["EURUSD"])], ["M1"], - count=1, + **kwargs, # type: ignore[arg-type] ) def test_skips_extra_fetch_when_start_pos_nonzero( @@ -1644,55 +1704,34 @@ class TestCollectLatestClosedRatesForAccounts: ) pd.testing.assert_frame_equal(result["EURUSD", 1], df_rate) - def test_rejects_zero_count_before_fetching(self, mocker: MockerFixture) -> None: - """Test count=0 is rejected before any MT5 collection attempt.""" - wrapped = mocker.patch( - "mt5cli.sdk.collect_latest_rates_for_accounts_with_retries", - ) - - with pytest.raises(ValueError, match="count must be positive"): - collect_latest_closed_rates_for_accounts( - [AccountSpec(symbols=["EURUSD"])], - ["M1"], - count=0, - ) - - wrapped.assert_not_called() - - def test_rejects_negative_start_pos(self, mocker: MockerFixture) -> None: - """Test negative start_pos is rejected before any MT5 collection attempt.""" - wrapped = mocker.patch( - "mt5cli.sdk.collect_latest_rates_for_accounts_with_retries", - ) - - with pytest.raises(ValueError, match="start_pos must be non-negative"): - collect_latest_closed_rates_for_accounts( - [AccountSpec(symbols=["EURUSD"])], - ["M1"], - count=1, - start_pos=-1, - ) - - wrapped.assert_not_called() - - def test_rejects_empty_frames_with_start_pos_nonzero( + @pytest.mark.parametrize( + ("kwargs", "match"), + [ + ({"count": 0}, "count must be positive"), + ({"count": 1, "start_pos": -1}, "start_pos must be non-negative"), + ], + ids=["zero-count", "negative-start-pos"], + ) + def test_rejects_invalid_inputs_before_fetching( self, mocker: MockerFixture, + kwargs: dict[str, object], + match: str, ) -> None: - """Test empty upstream frames raise ValueError when start_pos > 0.""" - mocker.patch( + """Test invalid count/start_pos values are rejected before MT5 is called.""" + wrapped = mocker.patch( "mt5cli.sdk.collect_latest_rates_for_accounts_with_retries", - return_value={("EURUSD", 1): pd.DataFrame(columns=["time", "close"])}, ) - with pytest.raises(ValueError, match="Rate data is empty"): + with pytest.raises(ValueError, match=match): collect_latest_closed_rates_for_accounts( [AccountSpec(symbols=["EURUSD"])], ["M1"], - count=1, - start_pos=1, + **kwargs, # type: ignore[arg-type] ) + wrapped.assert_not_called() + def test_processes_multiple_symbol_timeframe_pairs( self, mocker: MockerFixture, @@ -1816,104 +1855,114 @@ class TestCollectLatestClosedRatesByGranularity: class TestSubstituteEnvPlaceholders: """Tests for ${ENV_VAR} substitution.""" - def test_substitutes_known_variables( + @pytest.mark.parametrize( + ("env", "input_", "allow_whole_dollar_env", "expected"), + [ + pytest.param( + {"MT5_LOGIN": "12345", "MT5_SERVER": "Broker-Demo"}, + "${MT5_LOGIN}", + False, + "12345", + id="brace-substitution", + ), + pytest.param( + {"MT5_LOGIN": "12345", "MT5_SERVER": "Broker-Demo"}, + "srv=${MT5_SERVER}!", + False, + "srv=Broker-Demo!", + id="brace-substitution-embedded", + ), + pytest.param( + {}, + "plain", + False, + "plain", + id="plain-string-unchanged", + ), + pytest.param( + {"MT5_PASSWORD": "secret"}, + "$MT5_PASSWORD", + False, + "$MT5_PASSWORD", + id="whole-dollar-not-substituted-by-default", + ), + pytest.param( + {"MT5_PASSWORD": "secret"}, + "$MT5_PASSWORD", + True, + "secret", + id="whole-dollar-substituted-with-opt-in", + ), + pytest.param( + {"pass": "secret", "ENV": "val"}, + "plan$pass", + True, + "plan$pass", + id="partial-dollar-prefix-not-expanded", + ), + pytest.param( + {"pass": "secret", "ENV": "val"}, + "abc$ENV", + True, + "abc$ENV", + id="partial-env-suffix-not-expanded", + ), + pytest.param( + {"ENV": "val"}, + "$ENV-suffix", + True, + "$ENV-suffix", + id="whole-dollar-with-suffix-not-expanded", + ), + pytest.param( + {"MT5_LOGIN": "12345"}, + "${MT5_LOGIN}", + True, + "12345", + id="brace-substitution-with-opt-in", + ), + ], + ) + def test_substitute_env_placeholders( self, monkeypatch: pytest.MonkeyPatch, + env: dict[str, str], + input_: str, + allow_whole_dollar_env: bool, + expected: str, ) -> None: - """Test placeholders are replaced with environment values.""" - monkeypatch.setenv("MT5_LOGIN", "12345") - monkeypatch.setenv("MT5_SERVER", "Broker-Demo") + """Handle ${ENV}, $ENV, plain, and partial forms of substitution.""" + for name, value in env.items(): + monkeypatch.setenv(name, value) - assert substitute_env_placeholders("${MT5_LOGIN}") == "12345" - assert substitute_env_placeholders("srv=${MT5_SERVER}!") == "srv=Broker-Demo!" + result = substitute_env_placeholders( + input_, + allow_whole_dollar_env=allow_whole_dollar_env, + ) - def test_returns_plain_strings_unchanged(self) -> None: - """Test strings without placeholders are returned as-is.""" - assert substitute_env_placeholders("plain") == "plain" + assert result == expected - def test_raises_on_missing_variable( + @pytest.mark.parametrize( + ("input_", "allow_whole_dollar_env"), + [ + pytest.param("${MT5_MISSING}", False, id="brace-missing"), + pytest.param("$MT5_MISSING", True, id="whole-dollar-missing"), + ], + ) + def test_substitute_env_placeholders_raises_on_missing_env( self, monkeypatch: pytest.MonkeyPatch, + input_: str, + allow_whole_dollar_env: bool, ) -> None: - """Test a missing environment variable raises a clear error.""" + """Missing env vars raise ValueError for both ${ENV} and $ENV (opt-in) forms.""" monkeypatch.delenv("MT5_MISSING", raising=False) with pytest.raises(ValueError, match="'MT5_MISSING' is not set"): - substitute_env_placeholders("${MT5_MISSING}") - - def test_whole_dollar_not_substituted_by_default( - self, - monkeypatch: pytest.MonkeyPatch, - ) -> None: - """Test $ENV_NAME is not expanded without allow_whole_dollar_env=True.""" - monkeypatch.setenv("MT5_PASSWORD", "secret") - - assert substitute_env_placeholders("$MT5_PASSWORD") == "$MT5_PASSWORD" - - def test_whole_dollar_substituted_with_opt_in( - self, - monkeypatch: pytest.MonkeyPatch, - ) -> None: - """Test $ENV_NAME is expanded when allow_whole_dollar_env=True.""" - monkeypatch.setenv("MT5_PASSWORD", "secret") - - result = substitute_env_placeholders( - "$MT5_PASSWORD", allow_whole_dollar_env=True - ) - - assert result == "secret" - - def test_whole_dollar_missing_variable_raises_value_error( - self, - monkeypatch: pytest.MonkeyPatch, - ) -> None: - """Test missing $ENV_NAME raises ValueError when opt-in is enabled.""" - monkeypatch.delenv("MT5_MISSING", raising=False) - - with pytest.raises(ValueError, match="'MT5_MISSING' is not set"): - substitute_env_placeholders("$MT5_MISSING", allow_whole_dollar_env=True) - - def test_partial_dollar_not_expanded_with_opt_in( - self, - monkeypatch: pytest.MonkeyPatch, - ) -> None: - """Test $ENV embedded in a larger string is not expanded.""" - monkeypatch.setenv("pass", "secret") - monkeypatch.setenv("ENV", "val") - - assert ( - substitute_env_placeholders("plan$pass", allow_whole_dollar_env=True) - == "plan$pass" - ) - assert ( - substitute_env_placeholders("abc$ENV", allow_whole_dollar_env=True) - == "abc$ENV" - ) - - def test_dollar_with_suffix_not_expanded_with_opt_in( - self, - monkeypatch: pytest.MonkeyPatch, - ) -> None: - """Test $ENV-suffix is not expanded (not a whole-value placeholder).""" - monkeypatch.setenv("ENV", "val") - - assert ( - substitute_env_placeholders("$ENV-suffix", allow_whole_dollar_env=True) - == "$ENV-suffix" - ) - - def test_brace_format_works_with_opt_in( - self, - monkeypatch: pytest.MonkeyPatch, - ) -> None: - """Test ${ENV_VAR} substitution still works when allow_whole_dollar_env=True.""" - monkeypatch.setenv("MT5_LOGIN", "12345") - - result = substitute_env_placeholders( - "${MT5_LOGIN}", allow_whole_dollar_env=True - ) - - assert result == "12345" + substitute_env_placeholders( + input_, + allow_whole_dollar_env=allow_whole_dollar_env, + ) class TestResolveAccountSpec: @@ -2458,35 +2507,32 @@ class TestThrottledHistoryUpdater: assert updater.last_update_monotonic is None - def test_custom_backend_suppresses_recoverable_errors_when_requested( + @pytest.mark.parametrize( + ("suppress_errors", "raises"), + [ + (True, None), + (False, Mt5RuntimeError), + ], + ids=["suppress", "propagate"], + ) + def test_custom_backend_error_suppression( self, mocker: MockerFixture, + suppress_errors: bool, + raises: type[BaseException] | None, ) -> None: - """Test suppress_errors swallows recoverable custom backend errors.""" + """suppress_errors controls whether recoverable backend errors propagate.""" backend = mocker.Mock(side_effect=Mt5RuntimeError("boom")) updater = ThrottledHistoryUpdater( output="history.db", - suppress_errors=True, + suppress_errors=suppress_errors, update_backend=backend, ) - - assert updater.update(MagicMock(), ["EURUSD"]) is False - assert updater.last_update_monotonic is None - - def test_custom_backend_propagates_errors_when_not_suppressed( - self, - mocker: MockerFixture, - ) -> None: - """Test recoverable custom backend errors propagate by default.""" - backend = mocker.Mock(side_effect=Mt5RuntimeError("boom")) - updater = ThrottledHistoryUpdater( - output="history.db", - update_backend=backend, - ) - - with pytest.raises(Mt5RuntimeError, match="boom"): - updater.update(MagicMock(), ["EURUSD"]) - + if raises is None: + assert updater.update(MagicMock(), ["EURUSD"]) is False + else: + with pytest.raises(raises, match="boom"): + updater.update(MagicMock(), ["EURUSD"]) assert updater.last_update_monotonic is None @@ -2496,12 +2542,13 @@ class TestBuildConfigStringLogin: @pytest.mark.parametrize( ("login", "expected"), [ - (None, None), - (12345, 12345), - ("12345", 12345), - (" 12345 ", 12345), - ("", None), - (" ", None), + pytest.param(None, None, id="none-passthrough"), + pytest.param(12345, 12345, id="int-passthrough"), + pytest.param(54321, 54321, id="int-login-backward-compat"), + pytest.param("12345", 12345, id="numeric-string-coerced"), + pytest.param(" 12345 ", 12345, id="whitespace-padded-string-coerced"), + pytest.param("", None, id="empty-string-becomes-none"), + pytest.param(" ", None, id="whitespace-only-string-becomes-none"), ], ) def test_coerces_login_from_string( @@ -2509,7 +2556,10 @@ class TestBuildConfigStringLogin: login: int | str | None, expected: int | None, ) -> None: - """Test build_config coerces string login to int or None.""" + """Test build_config coerces string login to int/None. + + Int and None logins are left unchanged. + """ config = build_config(login=login) assert config.login == expected @@ -2518,23 +2568,29 @@ class TestBuildConfigStringLogin: with pytest.raises(ValueError, match="invalid literal"): build_config(login="abc") - def test_expands_dollar_brace_login_with_opt_in( + @pytest.mark.parametrize( + ("login_template", "env_value", "expected"), + [ + pytest.param("${MT5_LOGIN}", "12345", 12345, id="dollar-brace-expands"), + pytest.param("$MT5_LOGIN", "99999", 99999, id="whole-dollar-expands"), + pytest.param("${MT5_LOGIN}", "", None, id="blank-env-becomes-none"), + ], + ) + def test_expands_login_env_placeholder_with_opt_in( self, monkeypatch: pytest.MonkeyPatch, + login_template: str, + env_value: str, + expected: int | None, ) -> None: - """Test build_config expands ${MT5_LOGIN} and coerces with opt-in.""" - monkeypatch.setenv("MT5_LOGIN", "12345") - config = build_config(login="${MT5_LOGIN}", allow_whole_dollar_env=True) - assert config.login == 12345 + """Test build_config expands env-placeholder logins with opt-in. - def test_expands_whole_dollar_login_with_opt_in( - self, - monkeypatch: pytest.MonkeyPatch, - ) -> None: - """Test build_config expands $MT5_LOGIN and coerces with opt-in.""" - monkeypatch.setenv("MT5_LOGIN", "99999") - config = build_config(login="$MT5_LOGIN", allow_whole_dollar_env=True) - assert config.login == 99999 + Both ``${VAR}`` and whole-``$VAR`` syntax are expanded and coerced + when allow_whole_dollar_env=True; a blank expansion coerces to None. + """ + monkeypatch.setenv("MT5_LOGIN", env_value) + config = build_config(login=login_template, allow_whole_dollar_env=True) + assert config.login == expected def test_missing_env_variable_raises( self, @@ -2545,30 +2601,11 @@ class TestBuildConfigStringLogin: with pytest.raises(ValueError, match="'MT5_LOGIN' is not set"): build_config(login="${MT5_LOGIN}", allow_whole_dollar_env=True) - def test_env_expands_to_blank_becomes_none( - self, - monkeypatch: pytest.MonkeyPatch, - ) -> None: - """Test build_config coerces blank env-expanded login to None.""" - monkeypatch.setenv("MT5_LOGIN", "") - config = build_config(login="${MT5_LOGIN}", allow_whole_dollar_env=True) - assert config.login is None - def test_dollar_brace_login_not_expanded_without_opt_in(self) -> None: """Test ${MT5_LOGIN} is not expanded when allow_whole_dollar_env=False.""" with pytest.raises(ValueError, match="invalid literal"): build_config(login="${MT5_LOGIN}") - def test_integer_login_preserved_backward_compat(self) -> None: - """Test existing int login callers remain backward-compatible.""" - config = build_config(login=54321) - assert config.login == 54321 - - def test_none_login_preserved_backward_compat(self) -> None: - """Test existing None login callers remain backward-compatible.""" - config = build_config(login=None) - assert config.login is None - class TestSubstituteMappingValues: """Tests for substitute_mapping_values() (issue #62).""" @@ -2636,29 +2673,31 @@ class TestSubstituteMappingValues: ] } - def test_whole_dollar_expanded_with_opt_in( + @pytest.mark.parametrize( + ("allow_whole_dollar_env", "expected"), + [ + pytest.param(None, "$MT5_PASSWORD", id="default-preserves-whole-dollar"), + pytest.param(True, "secret", id="opt-in-expands-whole-dollar"), + ], + ) + def test_whole_dollar_env_handling( self, monkeypatch: pytest.MonkeyPatch, + allow_whole_dollar_env: bool | None, + expected: str, ) -> None: - """Test $ENV_NAME is expanded when allow_whole_dollar_env=True.""" + """Test $ENV_NAME handling for selected mapping keys.""" monkeypatch.setenv("MT5_PASSWORD", "secret") data: dict[str, object] = {"mt5_password": "$MT5_PASSWORD"} - result = substitute_mapping_values( - data, - keys={"mt5_password"}, - allow_whole_dollar_env=True, - ) - assert result == {"mt5_password": "secret"} - - def test_whole_dollar_not_expanded_by_default( - self, - monkeypatch: pytest.MonkeyPatch, - ) -> None: - """Test $ENV_NAME in a selected key is preserved when opt-in is False.""" - monkeypatch.setenv("MT5_PASSWORD", "secret") - data: dict[str, object] = {"mt5_password": "$MT5_PASSWORD"} - result = substitute_mapping_values(data, keys={"mt5_password"}) - assert result == {"mt5_password": "$MT5_PASSWORD"} + if allow_whole_dollar_env is None: + result = substitute_mapping_values(data, keys={"mt5_password"}) + else: + result = substitute_mapping_values( + data, + keys={"mt5_password"}, + allow_whole_dollar_env=allow_whole_dollar_env, + ) + assert result == {"mt5_password": expected} def test_blank_string_becomes_none_for_blank_keys(self) -> None: """Test blank strings are normalised to None for blank_string_keys_as_none.""" @@ -2831,35 +2870,29 @@ class TestUpdateObservability: row = conn.execute("SELECT status FROM snapshot_runs").fetchone() assert row == ("error",) - def test_update_observability_skips_ensure_grafana_schema_when_disabled( + @pytest.mark.parametrize( + ("with_grafana_schema", "expected_call_count"), + [ + pytest.param(False, 0, id="grafana-schema-disabled"), + pytest.param(True, 1, id="grafana-schema-enabled"), + ], + ) + def test_update_observability_grafana_schema_gate( self, mock_client: MagicMock, mocker: MockerFixture, tmp_path: Path, + with_grafana_schema: bool, + expected_call_count: int, ) -> None: - """with_grafana_schema=False does not call ensure_grafana_schema.""" + """with_grafana_schema controls whether ensure_grafana_schema is called.""" spy = mocker.spy(sdk, "ensure_grafana_schema") update_observability( client=mock_client, output=tmp_path / "obs.db", - with_grafana_schema=False, + with_grafana_schema=with_grafana_schema, ) - spy.assert_not_called() - - def test_update_observability_calls_ensure_grafana_schema_by_default( - self, - mock_client: MagicMock, - mocker: MockerFixture, - tmp_path: Path, - ) -> None: - """with_grafana_schema=True calls ensure_grafana_schema.""" - spy = mocker.spy(sdk, "ensure_grafana_schema") - update_observability( - client=mock_client, - output=tmp_path / "obs.db", - with_grafana_schema=True, - ) - spy.assert_called_once() + assert spy.call_count == expected_call_count @pytest.mark.parametrize( ("kwarg", "method"), @@ -2885,103 +2918,109 @@ class TestUpdateObservability: ) getattr(mock_client, method).assert_not_called() - def test_update_observability_with_positions_rows( + @pytest.mark.parametrize( + ("method", "table", "row", "expected_count"), + [ + ( + "positions_get_as_df", + "position_snapshots", + { + "ticket": 1, + "position_id": 1, + "symbol": "EURUSD", + "type": 0, + "volume": 0.1, + "price_open": 1.1, + "price_current": 1.1, + "profit": 0.0, + "swap": 0.0, + "comment": "", + "magic": 0, + }, + 1, + ), + ( + "orders_get_as_df", + "order_snapshots", + { + "ticket": 10, + "symbol": "EURUSD", + "type": 2, + "volume_current": 0.1, + "price_open": 1.2, + "price_current": 1.1, + "state": 1, + "comment": "", + "magic": 0, + "time_setup": 1700000000, + }, + 1, + ), + ], + ids=["positions", "orders"], + ) + def test_update_observability_writes_snapshot_rows( self, mock_client: MagicMock, tmp_path: Path, + method: str, + table: str, + row: dict[str, object], + expected_count: int, ) -> None: - """Non-empty positions are written to position_snapshots.""" - mock_client.positions_get_as_df.return_value = pd.DataFrame([ - { - "ticket": 1, - "position_id": 1, - "symbol": "EURUSD", - "type": 0, - "volume": 0.1, - "price_open": 1.1, - "price_current": 1.1, - "profit": 0.0, - "swap": 0.0, - "comment": "", - "magic": 0, - } - ]) + """Non-empty snapshots are written to the corresponding snapshot table.""" + getattr(mock_client, method).return_value = pd.DataFrame([row]) output = tmp_path / "obs.db" update_observability(client=mock_client, output=output) with sqlite3.connect(output) as conn: - count = conn.execute("SELECT COUNT(*) FROM position_snapshots").fetchone()[ - 0 - ] - assert count == 1 + count = conn.execute( + f"SELECT COUNT(*) FROM {table}" # noqa: S608 + ).fetchone()[0] + assert count == expected_count - def test_update_observability_with_order_rows( - self, - mock_client: MagicMock, - tmp_path: Path, - ) -> None: - """Non-empty orders are written to order_snapshots.""" - mock_client.orders_get_as_df.return_value = pd.DataFrame([ - { - "ticket": 10, - "symbol": "EURUSD", - "type": 2, - "volume_current": 0.1, - "price_open": 1.2, - "price_current": 1.1, - "state": 1, - "comment": "", - "magic": 0, - "time_setup": 1700000000, - } - ]) - output = tmp_path / "obs.db" - update_observability(client=mock_client, output=output) - with sqlite3.connect(output) as conn: - count = conn.execute("SELECT COUNT(*) FROM order_snapshots").fetchone()[0] - assert count == 1 - - def test_update_observability_symbol_filter_positions( + @pytest.mark.parametrize( + ("method", "table", "rows"), + [ + ( + "positions_get_as_df", + "position_snapshots", + [ + {"ticket": 1, "symbol": "EURUSD", "volume": 0.1, "profit": 0.0}, + {"ticket": 2, "symbol": "USDJPY", "volume": 0.2, "profit": 0.0}, + ], + ), + ( + "orders_get_as_df", + "order_snapshots", + [ + {"ticket": 10, "symbol": "EURUSD", "volume_current": 0.1}, + {"ticket": 11, "symbol": "USDJPY", "volume_current": 0.5}, + ], + ), + ], + ids=["positions", "orders"], + ) + def test_update_observability_symbol_filter( self, tmp_path: Path, + method: str, + table: str, + rows: list[dict[str, object]], ) -> None: - """Symbol filter fetches all positions in one call and filters client-side.""" - client = MagicMock() - client.account_info_as_df.return_value = pd.DataFrame([{"login": 1}]) - # All positions; only EURUSD matches the filter - client.positions_get_as_df.return_value = pd.DataFrame([ - {"ticket": 1, "symbol": "EURUSD", "volume": 0.1, "profit": 0.0}, - {"ticket": 2, "symbol": "USDJPY", "volume": 0.2, "profit": 0.0}, - ]) - client.orders_get_as_df.return_value = pd.DataFrame() - client.terminal_info_as_df.return_value = pd.DataFrame() - output = tmp_path / "obs.db" - update_observability(client=client, output=output, symbols=["EURUSD", "GBPUSD"]) - assert client.positions_get_as_df.call_count == 1 - with sqlite3.connect(output) as conn: - count = conn.execute("SELECT COUNT(*) FROM position_snapshots").fetchone()[ - 0 - ] - assert count == 1 - - def test_update_observability_symbol_filter_orders( - self, - tmp_path: Path, - ) -> None: - """Symbol filter fetches all orders in one call and filters client-side.""" + """Symbol filter fetches all rows in one call and filters client-side.""" client = MagicMock() client.account_info_as_df.return_value = pd.DataFrame([{"login": 1}]) client.positions_get_as_df.return_value = pd.DataFrame() - # All orders; only EURUSD matches the filter - client.orders_get_as_df.return_value = pd.DataFrame([ - {"ticket": 10, "symbol": "EURUSD", "volume_current": 0.1}, - {"ticket": 11, "symbol": "USDJPY", "volume_current": 0.5}, - ]) + client.orders_get_as_df.return_value = pd.DataFrame() client.terminal_info_as_df.return_value = pd.DataFrame() + getattr(client, method).return_value = pd.DataFrame(rows) output = tmp_path / "obs.db" update_observability(client=client, output=output, symbols=["EURUSD", "GBPUSD"]) - assert client.orders_get_as_df.call_count == 1 + assert getattr(client, method).call_count == 1 with sqlite3.connect(output) as conn: - count = conn.execute("SELECT COUNT(*) FROM order_snapshots").fetchone()[0] + count = conn.execute( + f"SELECT COUNT(*) FROM {table}" # noqa: S608 + ).fetchone()[0] assert count == 1 def test_update_observability_symbol_filter_no_symbol_col( @@ -3022,36 +3061,40 @@ class TestUpdateObservability: assert row is not None assert row[0] is None - def test_update_observability_empty_account_logs_warning( + @pytest.mark.parametrize( + ("method", "table", "message"), + [ + ( + "account_info_as_df", + "account_snapshots", + "account_info_as_df returned empty frame", + ), + ( + "terminal_info_as_df", + "terminal_snapshots", + "terminal_info_as_df returned empty frame", + ), + ], + ids=["account", "terminal"], + ) + def test_update_observability_empty_logs_warning( self, mock_client: MagicMock, tmp_path: Path, caplog: pytest.LogCaptureFixture, + method: str, + table: str, + message: str, ) -> None: - """Empty account_info_as_df logs a warning and does not write account row.""" - mock_client.account_info_as_df.return_value = pd.DataFrame() + """Empty snapshot frames log a warning and write no rows.""" + getattr(mock_client, method).return_value = pd.DataFrame() with caplog.at_level(logging.WARNING, logger="mt5cli.sdk"): update_observability(client=mock_client, output=tmp_path / "obs.db") - assert "account_info_as_df returned empty frame" in caplog.text + assert message in caplog.text with sqlite3.connect(tmp_path / "obs.db") as conn: - count = conn.execute("SELECT COUNT(*) FROM account_snapshots").fetchone()[0] - assert count == 0 - - def test_update_observability_empty_terminal_logs_warning( - self, - mock_client: MagicMock, - tmp_path: Path, - caplog: pytest.LogCaptureFixture, - ) -> None: - """Empty terminal_info_as_df logs a warning and does not write terminal row.""" - mock_client.terminal_info_as_df.return_value = pd.DataFrame() - with caplog.at_level(logging.WARNING, logger="mt5cli.sdk"): - update_observability(client=mock_client, output=tmp_path / "obs.db") - assert "terminal_info_as_df returned empty frame" in caplog.text - with sqlite3.connect(tmp_path / "obs.db") as conn: - count = conn.execute("SELECT COUNT(*) FROM terminal_snapshots").fetchone()[ - 0 - ] + count = conn.execute( + f"SELECT COUNT(*) FROM {table}" # noqa: S608 + ).fetchone()[0] assert count == 0 def test_update_observability_with_config_opens_and_closes_connection( diff --git a/tests/test_telemetry.py b/tests/test_telemetry.py index 2861283..42495ce 100644 --- a/tests/test_telemetry.py +++ b/tests/test_telemetry.py @@ -20,23 +20,13 @@ from mt5cli.telemetry import ( class TestNoOp: """Tests for _NoOp no-op instrument.""" - def test_add_is_noop(self) -> None: - """_NoOp.add accepts amount and optional attributes without error.""" + @pytest.mark.parametrize("method", ["add", "set", "record"]) + def test_method_is_noop(self, method: str) -> None: + """_NoOp methods accept amount and optional attributes without error.""" noop = _NoOp() - noop.add(1.0) - noop.add(1.0, {"key": "val"}) - - def test_set_is_noop(self) -> None: - """_NoOp.set accepts amount and optional attributes without error.""" - noop = _NoOp() - noop.set(2.0) - noop.set(2.0, {"key": "val"}) - - def test_record_is_noop(self) -> None: - """_NoOp.record accepts amount and optional attributes without error.""" - noop = _NoOp() - noop.record(3.0) - noop.record(3.0, {"key": "val"}) + bound = getattr(noop, method) + bound(1.0) + bound(1.0, {"key": "val"}) class TestMt5Metrics: @@ -64,32 +54,105 @@ class TestMt5Metrics: assert meter.create_counter.called assert meter.create_gauge.called - def test_record_history_update_success(self) -> None: - """record_history_update records duration and timestamp on success.""" + @pytest.mark.parametrize( + ( + "method", + "kwargs", + "duration_attr", + "failures_attr", + "last_success_attr", + ), + [ + pytest.param( + "record_history_update", + {"dataset": "rates"}, + "_history_duration", + "_history_failures", + "_last_successful_update", + id="history-update", + ), + pytest.param( + "record_snapshot_update", + {}, + "_snapshot_duration", + "_snapshot_failures", + None, + id="snapshot-update", + ), + ], + ) + def test_record_update_success( + self, + method: str, + kwargs: dict[str, str], + duration_attr: str, + failures_attr: str, + last_success_attr: str | None, + ) -> None: + """record_*_update records duration and timestamp on success.""" meter = MagicMock() m = _Mt5Metrics() m.configure(meter) - with m.record_history_update(dataset="rates"): + with getattr(m, method)(**kwargs): pass - m._history_duration.record.assert_called_once() # type: ignore[reportPrivateUsage] - m._last_successful_update.set.assert_called_once() # type: ignore[reportPrivateUsage] - m._history_failures.add.assert_not_called() # type: ignore[reportPrivateUsage] + getattr(m, duration_attr).record.assert_called_once() # type: ignore[reportPrivateUsage] + getattr(m, failures_attr).add.assert_not_called() # type: ignore[reportPrivateUsage] + if last_success_attr is not None: + getattr(m, last_success_attr).set.assert_called_once() # type: ignore[reportPrivateUsage] - def test_record_history_update_failure(self) -> None: - """record_history_update increments failure counter and re-raises on error.""" + @pytest.mark.parametrize( + ( + "method", + "kwargs", + "exc", + "duration_attr", + "failures_attr", + "failure_labels", + ), + [ + pytest.param( + "record_history_update", + {"dataset": "rates"}, + ValueError("boom"), + "_history_duration", + "_history_failures", + {"dataset": "rates"}, + id="history-update", + ), + pytest.param( + "record_snapshot_update", + {}, + RuntimeError("snap fail"), + "_snapshot_duration", + "_snapshot_failures", + {}, + id="snapshot-update", + ), + ], + ) + def test_record_update_failure( + self, + method: str, + kwargs: dict[str, str], + exc: BaseException, + duration_attr: str, + failures_attr: str, + failure_labels: dict[str, str], + ) -> None: + """record_*_update increments failure counter and re-raises on error.""" meter = MagicMock() m = _Mt5Metrics() m.configure(meter) - exc = ValueError("boom") with ( - pytest.raises(ValueError, match="boom"), - m.record_history_update(dataset="rates"), + pytest.raises(type(exc), match=str(exc)), + getattr(m, method)(**kwargs), ): raise exc - m._history_failures.add.assert_called_once_with( # type: ignore[reportPrivateUsage] - 1, {"dataset": "rates"} + getattr(m, failures_attr).add.assert_called_once_with( # type: ignore[reportPrivateUsage] + 1, + failure_labels, ) - m._history_duration.record.assert_not_called() # type: ignore[reportPrivateUsage] + getattr(m, duration_attr).record.assert_not_called() # type: ignore[reportPrivateUsage] def test_add_history_rows(self) -> None: """add_history_rows increments the rows-written counter.""" @@ -101,82 +164,82 @@ class TestMt5Metrics: 42, {"dataset": "rates"} ) - def test_record_snapshot_update_success(self) -> None: - """record_snapshot_update records duration on success.""" + @pytest.mark.parametrize( + ("method", "kwargs", "gauge_attr", "expected_set_count"), + [ + pytest.param( + "record_position_state", + { + "login": "42", + "server": "demo", + "symbol": "EURUSD", + "profit": 12.5, + "volume": 0.01, + }, + "_position_profit", + 2, + id="position-state", + ), + pytest.param( + "record_terminal_state", + { + "connected": 1.0, + "trade_allowed": 1.0, + "trade_expert": 0.0, + }, + "_terminal_connected", + 3, + id="terminal-state", + ), + pytest.param( + "record_account_state", + { + "login": "99", + "server": "live", + "balance": 5000.0, + "equity": 5100.0, + "margin": 200.0, + "margin_free": 4800.0, + "margin_level": 2550.0, + }, + "_account_balance", + 5, + id="account-state", + ), + ], + ) + def test_record_state_emits_gauges( + self, + method: str, + kwargs: dict[str, float | str], + gauge_attr: str, + expected_set_count: int, + ) -> None: + """record_*_state emits the expected gauge set calls after configure.""" meter = MagicMock() m = _Mt5Metrics() m.configure(meter) - with m.record_snapshot_update(): - pass - m._snapshot_duration.record.assert_called_once() # type: ignore[reportPrivateUsage] - m._snapshot_failures.add.assert_not_called() # type: ignore[reportPrivateUsage] - - def test_record_snapshot_update_failure(self) -> None: - """record_snapshot_update increments failure counter and re-raises on error.""" - meter = MagicMock() - m = _Mt5Metrics() - m.configure(meter) - exc = RuntimeError("snap fail") - with ( - pytest.raises(RuntimeError, match="snap fail"), - m.record_snapshot_update(), - ): - raise exc - m._snapshot_failures.add.assert_called_once_with(1, {}) # type: ignore[reportPrivateUsage] - m._snapshot_duration.record.assert_not_called() # type: ignore[reportPrivateUsage] - - def test_record_position_state(self) -> None: - """record_position_state emits profit and volume gauges.""" - meter = MagicMock() - m = _Mt5Metrics() - m.configure(meter) - m.record_position_state( - login="42", - server="demo", - symbol="EURUSD", - profit=12.5, - volume=0.01, + getattr(m, method)(**kwargs) + # Related gauges share the same create_gauge mock; verify total set calls. + assert ( # type: ignore[reportPrivateUsage] + getattr(m, gauge_attr).set.call_count == expected_set_count ) - # Both profit and volume share the same gauge mock via create_gauge. - # Verify that set was called exactly twice (once each). - assert m._position_profit.set.call_count == 2 # type: ignore[reportPrivateUsage] - def test_record_terminal_state(self) -> None: - """record_terminal_state emits connected, trade_allowed, trade_expert gauges.""" - meter = MagicMock() + @pytest.mark.parametrize( + ("method", "kwargs"), + [ + ("record_history_update", {"dataset": "ticks"}), + ("record_snapshot_update", {}), + ], + ) + def test_record_update_noop_before_configure( + self, + method: str, + kwargs: dict[str, str], + ) -> None: + """record_*_update works without configure (no-op instruments).""" m = _Mt5Metrics() - m.configure(meter) - m.record_terminal_state(connected=1.0, trade_allowed=1.0, trade_expert=0.0) - # All three terminal gauges share the same mock; set is called 3 times. - assert m._terminal_connected.set.call_count == 3 # type: ignore[reportPrivateUsage] - - def test_record_account_state_after_configure(self) -> None: - """record_account_state emits all five account gauges.""" - meter = MagicMock() - m = _Mt5Metrics() - m.configure(meter) - m.record_account_state( - login="99", - server="live", - balance=5000.0, - equity=5100.0, - margin=200.0, - margin_free=4800.0, - margin_level=2550.0, - ) - # All five account gauges share the same gauge mock; set is called 5 times. - assert m._account_balance.set.call_count == 5 # type: ignore[reportPrivateUsage] - - def test_record_history_update_noop_before_configure(self) -> None: - """record_history_update works without configure (no-op instruments).""" - m = _Mt5Metrics() - with m.record_history_update(dataset="ticks"): - pass - - def test_record_snapshot_update_noop_before_configure(self) -> None: - """record_snapshot_update works without configure (no-op instruments).""" - m = _Mt5Metrics() - with m.record_snapshot_update(): + with getattr(m, method)(**kwargs): pass diff --git a/tests/test_trading.py b/tests/test_trading.py index acd3ff1..9ed2796 100644 --- a/tests/test_trading.py +++ b/tests/test_trading.py @@ -21,6 +21,7 @@ from mt5cli.trading import ( MarginVolume, OrderExecutionResult, OrderLimits, + OrderSide, ProjectionMode, calculate_account_projected_margin_ratio, calculate_margin_and_volume, @@ -87,54 +88,31 @@ _MISSING_RETCODE: object = ( class TestDetectPositionSide: """Tests for detect_position_side.""" - def test_returns_none_when_no_positions(self) -> None: - """Test None is returned when no open positions exist.""" - client = MagicMock() - client.positions_get_as_df.return_value = pd.DataFrame() - - assert detect_position_side(client, "EURUSD") is None - - def test_returns_long_for_buy_only_exposure(self) -> None: - """Test long is returned when only buy positions exist.""" + @pytest.mark.parametrize( + ("types", "volumes", "expected"), + [ + ([], [], None), + ([0, 0], [0.2, 0.1], "long"), + ([1, 1], [0.3, 0.1], "short"), + ([0, 1], [0.3, 0.2], None), + ], + ids=["no-positions", "buy-only", "sell-only", "mixed"], + ) + def test_detect_position_side( + self, + types: list[int], + volumes: list[float], + expected: str | None, + ) -> None: + """detect_position_side reports net long/short/None from open positions.""" client = MagicMock() client.mt5.POSITION_TYPE_BUY = 0 client.mt5.POSITION_TYPE_SELL = 1 client.positions_get_as_df.return_value = pd.DataFrame( - { - "type": [0, 0], - "volume": [0.2, 0.1], - }, + {"type": types, "volume": volumes}, ) - assert detect_position_side(client, "EURUSD") == "long" - - def test_returns_short_for_net_sell_volume(self) -> None: - """Test short is returned when sell volume exceeds buy volume.""" - client = MagicMock() - client.mt5.POSITION_TYPE_BUY = 0 - client.mt5.POSITION_TYPE_SELL = 1 - client.positions_get_as_df.return_value = pd.DataFrame( - { - "type": [1, 1], - "volume": [0.3, 0.1], - }, - ) - - assert detect_position_side(client, "EURUSD") == "short" - - def test_returns_none_for_mixed_hedged_positions(self) -> None: - """Test None is returned for mixed buy and sell exposure.""" - client = MagicMock() - client.mt5.POSITION_TYPE_BUY = 0 - client.mt5.POSITION_TYPE_SELL = 1 - client.positions_get_as_df.return_value = pd.DataFrame( - { - "type": [0, 1], - "volume": [0.3, 0.2], - }, - ) - - assert detect_position_side(client, "EURUSD") is None + assert detect_position_side(client, "EURUSD") == expected class TestCalculateMarginAndVolume: @@ -175,9 +153,11 @@ class TestCalculateMarginAndVolume: ({"margin_free": 0.0}, 0.0), ({}, 0.0), ({"margin_free": None}, 0.0), + ({"margin_free": -500.0}, 0.0), ], + ids=["zero", "missing", "none", "negative"], ) - def test_zero_or_missing_margin_free( + def test_zero_missing_or_negative_margin_free( self, account_dict: dict[str, float | None], expected_margin_free: float, @@ -204,28 +184,6 @@ class TestCalculateMarginAndVolume: mock_calc_vol.assert_any_call(client, "EURUSD", 0.0, "BUY") mock_calc_vol.assert_any_call(client, "EURUSD", 0.0, "SELL") - def test_clamps_negative_margin_free_to_zero(self, mocker: MockerFixture) -> None: - """Test negative margin_free is clamped to zero before sizing.""" - client = MagicMock() - client.account_info_as_dict.return_value = {"margin_free": -500.0} - client.symbol_info_as_dict.side_effect = AttributeError("missing") - mock_calc_vol = mocker.patch( - "mt5cli.trading.calculate_volume_by_margin", return_value=0.0 - ) - - result = calculate_margin_and_volume( - client, - "EURUSD", - unit_margin_ratio=0.5, - preserved_margin_ratio=0.2, - ) - - _assert_close(result["margin_free"], 0.0) - _assert_close(result["buy_volume"], 0.0) - _assert_close(result["sell_volume"], 0.0) - mock_calc_vol.assert_any_call(client, "EURUSD", 0.0, "BUY") - mock_calc_vol.assert_any_call(client, "EURUSD", 0.0, "SELL") - @pytest.mark.parametrize( ("unit_ratio", "preserved_ratio"), [ @@ -286,8 +244,20 @@ class TestDetermineOrderLimits: assert result["stop_loss"] is None assert result["take_profit"] is None - def test_calculates_long_protective_levels(self) -> None: - """Test long stop loss and take profit are placed below/above entry.""" + @pytest.mark.parametrize( + ("side", "expected"), + [ + ("long", {"entry": 100.0, "stop_loss": 98.0, "take_profit": 103.0}), + ("short", {"entry": 99.0, "stop_loss": 100.98, "take_profit": 96.03}), + ], + ids=["long", "short"], + ) + def test_calculates_protective_levels( + self, + side: str, + expected: dict[str, float | None], + ) -> None: + """Test long/short stop loss and take profit are placed below/above entry.""" client = MagicMock() client.symbol_info_tick_as_dict.return_value = {"ask": 100.0, "bid": 99.0} client.symbol_info_as_dict.return_value = {} @@ -295,36 +265,12 @@ class TestDetermineOrderLimits: result = determine_order_limits( client, "EURUSD", - "long", + side, stop_loss_limit_ratio=0.02, take_profit_limit_ratio=0.03, ) - assert result == { - "entry": 100.0, - "stop_loss": 98.0, - "take_profit": 103.0, - } - - def test_calculates_short_protective_levels(self) -> None: - """Test short stop loss and take profit are placed above/below entry.""" - client = MagicMock() - client.symbol_info_tick_as_dict.return_value = {"ask": 100.0, "bid": 99.0} - client.symbol_info_as_dict.return_value = {} - - result = determine_order_limits( - client, - "EURUSD", - "short", - stop_loss_limit_ratio=0.02, - take_profit_limit_ratio=0.03, - ) - - assert result == { - "entry": 99.0, - "stop_loss": 100.98, - "take_profit": 96.03, - } + assert result == expected def test_rejects_unknown_side(self) -> None: """Test unsupported side values raise ValueError.""" @@ -596,24 +542,32 @@ class TestDetermineOrderLimits: """Tests for ensure_symbol_selected.""" - def test_skips_selection_when_symbol_is_visible(self) -> None: - """Test visible symbols do not call symbol_select.""" + @pytest.mark.parametrize( + ("visible", "select_result", "expected_calls"), + [ + (True, True, 0), + (False, True, 1), + ], + ids=["already-visible", "select-hidden"], + ) + def test_ensure_symbol_selected_success_cases( + self, + visible: bool, + select_result: bool, + expected_calls: int, + ) -> None: + """Test symbol selection only occurs when the symbol is hidden.""" client = MagicMock() - client.symbol_info_as_dict.return_value = {"visible": True} + client.symbol_info_as_dict.return_value = {"visible": visible} + client.symbol_select.return_value = select_result ensure_symbol_selected(client, "EURUSD") - client.symbol_select.assert_not_called() - - def test_selects_hidden_symbol_before_trading(self) -> None: - """Test hidden symbols are selected in Market Watch.""" - client = MagicMock() - client.symbol_info_as_dict.return_value = {"visible": False} - client.symbol_select.return_value = True - - ensure_symbol_selected(client, "EURUSD") - - client.symbol_select.assert_called_once_with("EURUSD", enable=True) + assert client.symbol_select.call_count == expected_calls + if expected_calls: + client.symbol_select.assert_called_once_with("EURUSD", enable=True) + else: + client.symbol_select.assert_not_called() def test_raises_when_symbol_selection_fails(self) -> None: """Test failed symbol selection raises Mt5TradingError.""" @@ -782,33 +736,18 @@ class TestSnapshotsAndState: assert "ticket" in result.columns assert "comment" in result.columns - def test_calculate_spread_ratio(self) -> None: - """Test spread ratio uses mid-price denominator.""" + @pytest.mark.parametrize( + "tick", + [ + {"bid": 99.0, "ask": 101.0}, + {"bid": "99.0", "ask": "101.0"}, + ], + ids=["numeric", "numeric-string"], + ) + def test_calculate_spread_ratio(self, tick: dict[str, object]) -> None: + """Test spread ratio uses mid-price denominator for numeric ticks.""" client = MagicMock() - client.symbol_info_tick_as_dict.return_value = {"bid": 99.0, "ask": 101.0} - - _assert_close(calculate_spread_ratio(client, "EURUSD"), 0.02) - - def test_calculate_spread_ratio_rejects_missing_tick(self) -> None: - """Test missing bid/ask raises a trading error.""" - client = MagicMock() - client.symbol_info_tick_as_dict.return_value = {"bid": None, "ask": 1.0} - - with pytest.raises(Mt5OperationError): - calculate_spread_ratio(client, "EURUSD") - - def test_calculate_spread_ratio_rejects_non_positive_tick(self) -> None: - """Test non-positive bid/ask raises a trading error.""" - client = MagicMock() - client.symbol_info_tick_as_dict.return_value = {"bid": 0.0, "ask": 1.0} - - with pytest.raises(Mt5OperationError): - calculate_spread_ratio(client, "EURUSD") - - def test_calculate_spread_ratio_accepts_numeric_string_tick(self) -> None: - """Test numeric string bid/ask values are accepted.""" - client = MagicMock() - client.symbol_info_tick_as_dict.return_value = {"bid": "99.0", "ask": "101.0"} + client.symbol_info_tick_as_dict.return_value = tick _assert_close(calculate_spread_ratio(client, "EURUSD"), 0.02) @@ -853,100 +792,48 @@ class TestSnapshotsAndState: class TestNormalizeOrderVolume: """Tests for normalize_order_volume.""" - def test_returns_exact_minimum_volume(self) -> None: - """Test exact volume_min is returned unchanged.""" + @pytest.mark.parametrize( + ("volume", "volume_min", "volume_max", "volume_step", "expected"), + [ + (0.1, 0.1, 1.0, 0.1, 0.1), + (0.25, 0.1, 1.0, 0.1, 0.2), + (0.9, 0.1, 0.5, 0.1, 0.5), + (0.05, 0.1, 1.0, 0.1, 0.0), + (1.0, 0.0, 1.0, 0.1, 0.0), + (1.0, 0.1, 1.0, 0.0, 0.0), + (2.5, 0.1, 0.0, 0.1, 2.5), + (0.5, 0.1, 0.34, 0.12, 0.34), + (2.5, 0.1, float("nan"), 0.1, 2.5), + ], + ids=[ + "exact-minimum", + "floor-to-step", + "clamp-to-max", + "below-minimum", + "invalid-volume-min", + "invalid-volume-step", + "non-positive-max-no-cap", + "max-reapplied-after-step", + "non-finite-max-no-cap", + ], + ) + def test_normalize_order_volume_deterministic( + self, + volume: float, + volume_min: float, + volume_max: float, + volume_step: float, + expected: float, + ) -> None: + """normalize_order_volume floors, clamps, and validates constraints.""" _assert_close( normalize_order_volume( - 0.1, - volume_min=0.1, - volume_max=1.0, - volume_step=0.1, + volume, + volume_min=volume_min, + volume_max=volume_max, + volume_step=volume_step, ), - 0.1, - ) - - def test_floors_to_step_between_boundaries(self) -> None: - """Test volume between steps floors down to the nearest valid step.""" - _assert_close( - normalize_order_volume( - 0.25, - volume_min=0.1, - volume_max=1.0, - volume_step=0.1, - ), - 0.2, - ) - - def test_clamps_to_volume_max(self) -> None: - """Test positive volume_max caps the normalized result.""" - _assert_close( - normalize_order_volume( - 0.9, - volume_min=0.1, - volume_max=0.5, - volume_step=0.1, - ), - 0.5, - ) - - def test_returns_zero_below_volume_min(self) -> None: - """Test sub-minimum requests return zero volume.""" - _assert_close( - normalize_order_volume( - 0.05, - volume_min=0.1, - volume_max=1.0, - volume_step=0.1, - ), - 0.0, - ) - - def test_returns_zero_for_invalid_volume_min(self) -> None: - """Test non-positive volume_min returns zero volume.""" - _assert_close( - normalize_order_volume( - 1.0, - volume_min=0.0, - volume_max=1.0, - volume_step=0.1, - ), - 0.0, - ) - - def test_returns_zero_for_invalid_volume_step(self) -> None: - """Test non-positive volume_step returns zero volume.""" - _assert_close( - normalize_order_volume( - 1.0, - volume_min=0.1, - volume_max=1.0, - volume_step=0.0, - ), - 0.0, - ) - - def test_treats_non_positive_volume_max_as_no_cap(self) -> None: - """Test volume_max <= 0 disables the maximum cap.""" - _assert_close( - normalize_order_volume( - 2.5, - volume_min=0.1, - volume_max=0.0, - volume_step=0.1, - ), - 2.5, - ) - - def test_reapplies_volume_max_after_step_normalization(self) -> None: - """Test post-step normalization cannot exceed volume_max.""" - _assert_close( - normalize_order_volume( - 0.5, - volume_min=0.1, - volume_max=0.34, - volume_step=0.12, - ), - 0.34, + expected, ) @pytest.mark.parametrize("volume", [float("nan"), float("inf")], ids=["nan", "inf"]) @@ -975,55 +862,41 @@ class TestNormalizeOrderVolume: 0.0, ) - def test_treats_non_finite_volume_max_as_no_cap(self) -> None: - """Test non-finite volume_max disables the maximum cap.""" - _assert_close( - normalize_order_volume( - 2.5, - volume_min=0.1, - volume_max=float("nan"), - volume_step=0.1, - ), - 2.5, - ) - class TestEstimateOrderMargin: """Tests for estimate_order_margin.""" - def test_estimates_buy_margin_at_ask(self) -> None: - """Test buy margin uses ask price and buy order type.""" + @pytest.mark.parametrize( + ("side", "order_type", "expected_price", "expected_margin"), + [ + ("BUY", 10, 1.1010, 12.5), + ("SELL", 11, 1.1000, 12.4), + ("long", 10, 1.1010, 12.5), + ("short", 11, 1.1000, 12.4), + ], + ids=["buy", "sell", "long", "short"], + ) + def test_estimates_margin_for_side( + self, + side: str, + order_type: int, + expected_price: float, + expected_margin: float, + ) -> None: + """Test estimate_order_margin uses ask for buy/long and bid for sell/short.""" client = _mock_trade_client() client.symbol_info_tick_as_dict.return_value = {"ask": 1.1010, "bid": 1.1000} - client.order_calc_margin.return_value = 12.5 + client.order_calc_margin.return_value = expected_margin - margin = estimate_order_margin(client, "EURUSD", "BUY", 0.1) + margin = estimate_order_margin(client, "EURUSD", side, 0.1) - _assert_close(margin, 12.5) - client.order_calc_margin.assert_called_once_with(10, "EURUSD", 0.1, 1.1010) - - def test_estimates_sell_margin_at_bid(self) -> None: - """Test sell margin uses bid price and sell order type.""" - client = _mock_trade_client() - client.symbol_info_tick_as_dict.return_value = {"ask": 1.1010, "bid": 1.1000} - client.order_calc_margin.return_value = 12.4 - - margin = estimate_order_margin(client, "EURUSD", "SELL", 0.1) - - _assert_close(margin, 12.4) - client.order_calc_margin.assert_called_once_with(11, "EURUSD", 0.1, 1.1000) - - def test_accepts_long_and_short_aliases(self) -> None: - """Test long/short aliases normalize to buy/sell pricing.""" - client = _mock_trade_client() - client.symbol_info_tick_as_dict.return_value = {"ask": 1.1010, "bid": 1.1000} - client.order_calc_margin.side_effect = [12.5, 12.4] - - estimate_order_margin(client, "EURUSD", "long", 0.1) - estimate_order_margin(client, "EURUSD", "short", 0.1) - - client.order_calc_margin.assert_any_call(10, "EURUSD", 0.1, 1.1010) - client.order_calc_margin.assert_any_call(11, "EURUSD", 0.1, 1.1000) + _assert_close(margin, expected_margin) + client.order_calc_margin.assert_called_once_with( + order_type, + "EURUSD", + 0.1, + expected_price, + ) def test_rejects_invalid_side(self) -> None: """Test unsupported order side raises ValueError.""" @@ -1032,16 +905,13 @@ class TestEstimateOrderMargin: with pytest.raises(ValueError, match="Unsupported order side"): estimate_order_margin(client, "EURUSD", "HOLD", 0.1) - def test_rejects_non_positive_volume(self) -> None: - """Test non-positive volume raises Mt5TradingError.""" - client = _mock_trade_client() - - with pytest.raises(Mt5OperationError, match="positive finite number"): - estimate_order_margin(client, "EURUSD", "BUY", 0.0) - - @pytest.mark.parametrize("volume", [float("nan"), float("inf")], ids=["nan", "inf"]) - def test_rejects_non_finite_volume(self, volume: float) -> None: - """Test NaN or infinite volume raises Mt5TradingError without broker calls.""" + @pytest.mark.parametrize( + "volume", + [0.0, float("nan"), float("inf")], + ids=["zero", "nan", "inf"], + ) + def test_rejects_invalid_volume(self, volume: float) -> None: + """Test non-positive or non-finite volume raises Mt5TradingError.""" client = _mock_trade_client() with pytest.raises(Mt5OperationError, match="positive finite number"): @@ -1050,29 +920,18 @@ class TestEstimateOrderMargin: client.symbol_info_tick_as_dict.assert_not_called() client.order_calc_margin.assert_not_called() - def test_rejects_missing_tick_prices(self) -> None: - """Test missing tick prices raise Mt5TradingError.""" + @pytest.mark.parametrize( + "ask", + [None, 0.0, float("inf")], + ids=["missing", "non-positive", "non-finite"], + ) + def test_rejects_invalid_tick_price( + self, + ask: float | None, + ) -> None: + """Test invalid ask prices raise Mt5OperationError.""" client = _mock_trade_client() - client.symbol_info_tick_as_dict.return_value = {"ask": None, "bid": 1.1000} - - with pytest.raises(Mt5OperationError, match="Tick price is unavailable"): - estimate_order_margin(client, "EURUSD", "BUY", 0.1) - - def test_rejects_non_positive_tick_price(self) -> None: - """Test non-positive tick prices raise Mt5TradingError.""" - client = _mock_trade_client() - client.symbol_info_tick_as_dict.return_value = {"ask": 0.0, "bid": 1.1000} - - with pytest.raises(Mt5OperationError, match="Tick price is unavailable"): - estimate_order_margin(client, "EURUSD", "BUY", 0.1) - - def test_rejects_non_finite_tick_price(self) -> None: - """Test non-finite tick prices raise Mt5TradingError.""" - client = _mock_trade_client() - client.symbol_info_tick_as_dict.return_value = { - "ask": float("inf"), - "bid": 1.1000, - } + client.symbol_info_tick_as_dict.return_value = {"ask": ask, "bid": 1.1000} with pytest.raises(Mt5OperationError, match="Tick price is unavailable"): estimate_order_margin(client, "EURUSD", "BUY", 0.1) @@ -1102,42 +961,6 @@ class TestCalculatePositionsMargin: _assert_close(calculate_positions_margin(client), 0.0) - def test_filters_by_symbols(self) -> None: - """Test optional symbol filter limits summed positions.""" - client = _mock_trade_client() - client.positions_get_as_df.return_value = pd.DataFrame( - [ - {"symbol": "EURUSD", "type": 0, "volume": 0.1}, - {"symbol": "USDJPY", "type": 1, "volume": 0.2}, - ], - ) - client.symbol_info_tick_as_dict.side_effect = [ - {"ask": 1.1010, "bid": 1.1000}, - {"ask": 110.0, "bid": 109.0}, - ] - client.order_calc_margin.side_effect = [12.5, 20.0] - - margin = calculate_positions_margin(client, symbols=["EURUSD"]) - - _assert_close(margin, 12.5) - assert client.order_calc_margin.call_count == 1 - - def test_sums_mixed_buy_and_sell_positions(self) -> None: - """Test mixed buy/sell exposure sums each side independently.""" - client = _mock_trade_client() - client.positions_get_as_df.return_value = pd.DataFrame( - [ - {"symbol": "EURUSD", "type": 0, "volume": 0.1}, - {"symbol": "EURUSD", "type": 1, "volume": 0.2}, - ], - ) - client.symbol_info_tick_as_dict.return_value = {"ask": 1.1010, "bid": 1.1000} - client.order_calc_margin.side_effect = [12.5, 24.8] - - margin = calculate_positions_margin(client) - - _assert_close(margin, 37.3) - def test_groups_positions_by_symbol_and_side(self) -> None: """Test repeated symbol/side pairs use one margin call with summed volume.""" client = _mock_trade_client() @@ -1160,24 +983,80 @@ class TestCalculatePositionsMargin: _assert_close(args[2], 0.3) _assert_close(args[3], 1.1010) - def test_sums_multiple_symbols(self) -> None: - """Test positions across symbols are all included.""" + @pytest.mark.parametrize( + ( + "positions_records", + "tick_records", + "margin_values", + "symbols", + "expected", + "expected_margin_calls", + ), + [ + ( + [ + {"symbol": "EURUSD", "type": 0, "volume": 0.1}, + {"symbol": "USDJPY", "type": 1, "volume": 0.2}, + ], + [ + {"ask": 1.1010, "bid": 1.1000}, + {"ask": 110.0, "bid": 109.0}, + ], + [12.5, 20.0], + ["EURUSD"], + 12.5, + 1, + ), + ( + [ + {"symbol": "EURUSD", "type": 0, "volume": 0.1}, + {"symbol": "EURUSD", "type": 1, "volume": 0.2}, + ], + [ + {"ask": 1.1010, "bid": 1.1000}, + {"ask": 1.1010, "bid": 1.1000}, + ], + [12.5, 24.8], + None, + 37.3, + 2, + ), + ( + [ + {"symbol": "EURUSD", "type": 0, "volume": 0.1}, + {"symbol": "GBPUSD", "type": 1, "volume": 0.3}, + ], + [ + {"ask": 1.1010, "bid": 1.1000}, + {"ask": 1.3010, "bid": 1.3000}, + ], + [12.5, 30.0], + None, + 42.5, + 2, + ), + ], + ids=["filters-by-symbol", "mixed-buy-sell", "multiple-symbols"], + ) + def test_sums_margin_for_normal_position_sets( + self, + positions_records: list[dict[str, object]], + tick_records: list[dict[str, float]], + margin_values: list[float], + symbols: list[str] | None, + expected: float, + expected_margin_calls: int, + ) -> None: + """Test standard valid position sets sum per-group margins correctly.""" client = _mock_trade_client() - client.positions_get_as_df.return_value = pd.DataFrame( - [ - {"symbol": "EURUSD", "type": 0, "volume": 0.1}, - {"symbol": "GBPUSD", "type": 1, "volume": 0.3}, - ], - ) - client.symbol_info_tick_as_dict.side_effect = [ - {"ask": 1.1010, "bid": 1.1000}, - {"ask": 1.3010, "bid": 1.3000}, - ] - client.order_calc_margin.side_effect = [12.5, 30.0] + client.positions_get_as_df.return_value = pd.DataFrame(positions_records) + client.symbol_info_tick_as_dict.side_effect = tick_records + client.order_calc_margin.side_effect = margin_values - margin = calculate_positions_margin(client) + margin = calculate_positions_margin(client, symbols=symbols) - _assert_close(margin, 42.5) + _assert_close(margin, expected) + assert client.order_calc_margin.call_count == expected_margin_calls def test_propagates_invalid_tick_or_margin_errors(self) -> None: """Test invalid tick or margin data raises Mt5TradingError.""" @@ -1190,35 +1069,30 @@ class TestCalculatePositionsMargin: with pytest.raises(Mt5OperationError, match="Tick price is unavailable"): calculate_positions_margin(client) - def test_skips_rows_with_invalid_symbol_volume_or_type(self) -> None: - """Test malformed position rows are ignored when summing margin.""" - client = _mock_trade_client() - client.positions_get_as_df.return_value = pd.DataFrame( + @pytest.mark.parametrize( + "positions_records", + [ [ {"symbol": "", "type": 0, "volume": 0.1}, {"symbol": "EURUSD", "type": 0, "volume": 0.0}, {"symbol": "EURUSD", "type": 2, "volume": 0.1}, {"symbol": "EURUSD", "type": 0, "volume": 0.1}, ], - ) - client.symbol_info_tick_as_dict.return_value = {"ask": 1.1010, "bid": 1.1000} - client.order_calc_margin.return_value = 12.5 - - margin = calculate_positions_margin(client) - - _assert_close(margin, 12.5) - client.order_calc_margin.assert_called_once() - - def test_skips_rows_with_non_finite_volume(self) -> None: - """Test NaN and infinite position volumes are ignored.""" - client = _mock_trade_client() - client.positions_get_as_df.return_value = pd.DataFrame( [ {"symbol": "EURUSD", "type": 0, "volume": float("nan")}, {"symbol": "EURUSD", "type": 0, "volume": float("inf")}, {"symbol": "EURUSD", "type": 0, "volume": 0.1}, ], - ) + ], + ids=["invalid-symbol-volume-type", "non-finite-volume"], + ) + def test_skips_invalid_rows_and_sums_remaining_valid_row( + self, + positions_records: list[dict[str, object]], + ) -> None: + """Test malformed or non-finite position rows are ignored when summing.""" + client = _mock_trade_client() + client.positions_get_as_df.return_value = pd.DataFrame(positions_records) client.symbol_info_tick_as_dict.return_value = {"ask": 1.1010, "bid": 1.1000} client.order_calc_margin.return_value = 12.5 @@ -1270,72 +1144,41 @@ class TestCalculatePositionsMargin: class TestVolumeAndExecution: """Tests for order planning and execution helpers.""" - def test_calculate_volume_by_margin_rounds_down_to_step(self) -> None: - """Test affordable volume respects min, max, and step.""" + @pytest.mark.parametrize( + ("volume_max", "margin_per_lot", "budget", "expected_volume"), + [ + (1.0, 25.0, 130.0, 0.5), + (0.3, 10.0, 100.0, 0.3), + (0.0, 10.0, 35.0, 0.3), + (1.0, 25.0, 10.0, 0.0), + ], + ids=[ + "rounds-down-to-step", + "caps-at-max-volume", + "ignores-zero-max-volume", + "returns-zero-when-unaffordable", + ], + ) + def test_calculate_volume_by_margin_boundary( + self, + volume_max: float, + margin_per_lot: float, + budget: float, + expected_volume: float, + ) -> None: + """Test volume caps, step rounding, and affordability under min/max/step.""" client = _mock_trade_client() client.symbol_info_as_dict.return_value = { "volume_min": 0.1, - "volume_max": 1.0, + "volume_max": volume_max, "volume_step": 0.1, } client.symbol_info_tick_as_dict.return_value = {"ask": 100.0, "bid": 99.0} - client.order_calc_margin.return_value = 25.0 + client.order_calc_margin.return_value = margin_per_lot _assert_close( - calculate_volume_by_margin( - client, - "EURUSD", - 130.0, - "BUY", - ), - 0.5, - ) - - def test_calculate_volume_by_margin_caps_at_max_volume(self) -> None: - """Test positive volume_max caps raw affordable volume exactly.""" - client = _mock_trade_client() - client.symbol_info_as_dict.return_value = { - "volume_min": 0.1, - "volume_max": 0.3, - "volume_step": 0.1, - } - client.symbol_info_tick_as_dict.return_value = {"ask": 100.0, "bid": 99.0} - client.order_calc_margin.return_value = 10.0 - - _assert_close(calculate_volume_by_margin(client, "EURUSD", 100.0, "BUY"), 0.3) - - def test_calculate_volume_by_margin_ignores_zero_max_volume(self) -> None: - """Test zero volume_max means uncapped volume normalization.""" - client = _mock_trade_client() - client.symbol_info_as_dict.return_value = { - "volume_min": 0.1, - "volume_max": 0.0, - "volume_step": 0.1, - } - client.symbol_info_tick_as_dict.return_value = {"ask": 100.0, "bid": 99.0} - client.order_calc_margin.return_value = 10.0 - - _assert_close(calculate_volume_by_margin(client, "EURUSD", 35.0, "BUY"), 0.3) - - def test_calculate_volume_by_margin_returns_zero_when_unaffordable(self) -> None: - """Test unaffordable minimum volume returns zero.""" - client = _mock_trade_client() - client.symbol_info_as_dict.return_value = { - "volume_min": 0.1, - "volume_max": 1.0, - "volume_step": 0.1, - } - client.symbol_info_tick_as_dict.return_value = {"ask": 100.0, "bid": 99.0} - client.order_calc_margin.return_value = 25.0 - - _assert_close( - calculate_volume_by_margin( - client, - "EURUSD", - 10.0, - "BUY", - ), - 0.0, + calculate_volume_by_margin(client, "EURUSD", budget, "BUY"), + expected_volume, ) def test_calculate_volume_by_margin_never_returns_nonzero_below_volume_min( @@ -1395,43 +1238,34 @@ class TestVolumeAndExecution: with pytest.raises(Mt5OperationError): calculate_volume_by_margin(client, "EURUSD", 100.0, "SELL") - def test_calculate_volume_by_margin_steps_down_when_margin_exceeds_budget( + @pytest.mark.parametrize( + ("volume_max", "budget", "margin_values", "expected"), + [ + (1.0, 130.0, [25.0, 75.0, 100.0, 150.0], 0.4), + (0.5, 130.0, [10.0, 150.0, 150.0], 0.0), + ], + ids=["steps-down-to-affordable-boundary", "all-steps-unaffordable"], + ) + def test_calculate_volume_by_margin_binary_search_affordability_boundaries( self, + volume_max: float, + budget: float, + margin_values: list[float], + expected: float, ) -> None: - """Tiered margin: binary search returns the largest affordable step.""" + """Binary search returns the largest affordable step or zero when none fit.""" client = _mock_trade_client() client.symbol_info_as_dict.return_value = { "volume_min": 0.1, - "volume_max": 1.0, + "volume_max": volume_max, "volume_step": 0.1, } client.symbol_info_tick_as_dict.return_value = {"ask": 100.0, "bid": 99.0} - # min_margin (0.1): 25.0 -> hi=4. - # Search: mid=2 (0.3)->75<=130, mid=3 (0.4)->100<=130, mid=4 (0.5)->150>130. - client.order_calc_margin.side_effect = [25.0, 75.0, 100.0, 150.0] + client.order_calc_margin.side_effect = margin_values - result = calculate_volume_by_margin(client, "EURUSD", 130.0, "BUY") + result = calculate_volume_by_margin(client, "EURUSD", budget, "BUY") - _assert_close(result, 0.4) - - def test_calculate_volume_by_margin_returns_zero_when_all_steps_unaffordable( - self, - ) -> None: - """If all binary-search probes exceed budget, returns zero.""" - client = _mock_trade_client() - client.symbol_info_as_dict.return_value = { - "volume_min": 0.1, - "volume_max": 0.5, - "volume_step": 0.1, - } - client.symbol_info_tick_as_dict.return_value = {"ask": 100.0, "bid": 99.0} - # min_margin (0.1): 10.0 -> hi=4. - # Search: mid=2 (0.3)->150>130->hi=1, mid=0 (0.1)->150>130->hi=-1. - client.order_calc_margin.side_effect = [10.0, 150.0, 150.0] - - result = calculate_volume_by_margin(client, "EURUSD", 130.0, "BUY") - - _assert_close(result, 0.0) + _assert_close(result, expected) def test_calculate_volume_by_margin_binary_search_is_bounded(self) -> None: """Binary search finds the largest affordable volume in O(log n) MT5 calls.""" @@ -1496,82 +1330,58 @@ class TestVolumeAndExecution: _assert_close(result["buy_volume"], 0.5) _assert_close(result["sell_volume"], 0.5) - def test_calculate_margin_and_volume_zero_ratio_uses_minimum_volume( + @pytest.mark.parametrize( + ( + "margin_free", + "preserved_margin_ratio", + "order_calc_margins", + "expected_available_margin", + "expected_buy_volume", + "expected_sell_volume", + ), + [ + (100.0, 0.0, [10.0, 20.0], 100.0, 0.1, 0.1), + (15.0, 0.0, [10.0, 20.0], 15.0, 0.1, 0.0), + (100.0, 0.9, [11.0, 9.0], 10.0, 0.0, 0.1), + ], + ids=[ + "uses-minimum-volume", + "rejects-unaffordable-side", + "preserves-margin-first", + ], + ) + def test_calculate_margin_and_volume_zero_ratio_sizes_minimum_lots( self, + margin_free: float, + preserved_margin_ratio: float, + order_calc_margins: list[float], + expected_available_margin: float, + expected_buy_volume: float, + expected_sell_volume: float, ) -> None: - """Test zero unit ratio requests one minimum volume when affordable.""" + """Test zero unit ratio sizes minimum lots against available margin.""" client = _mock_trade_client() - client.account_info_as_dict.return_value = {"margin_free": 100.0} + client.account_info_as_dict.return_value = {"margin_free": margin_free} client.symbol_info_as_dict.return_value = { "volume_min": 0.1, "volume_max": 1.0, "volume_step": 0.1, } client.symbol_info_tick_as_dict.return_value = {"ask": 100.0, "bid": 99.0} - client.order_calc_margin.side_effect = [10.0, 20.0] + client.order_calc_margin.side_effect = order_calc_margins result = calculate_margin_and_volume( client, "EURUSD", unit_margin_ratio=0.0, - preserved_margin_ratio=0.0, + preserved_margin_ratio=preserved_margin_ratio, ) - _assert_close(result["available_margin"], 100.0) - _assert_close(result["trade_margin"], 0.0) - _assert_close(result["buy_volume"], 0.1) - _assert_close(result["sell_volume"], 0.1) + _assert_close(result["available_margin"], expected_available_margin) + _assert_close(result["buy_volume"], expected_buy_volume) + _assert_close(result["sell_volume"], expected_sell_volume) client.calculate_volume_by_margin.assert_not_called() - def test_calculate_margin_and_volume_zero_ratio_rejects_unaffordable_side( - self, - ) -> None: - """Test zero unit ratio returns zero for unaffordable minimum lots.""" - client = _mock_trade_client() - client.account_info_as_dict.return_value = {"margin_free": 15.0} - client.symbol_info_as_dict.return_value = { - "volume_min": 0.1, - "volume_max": 1.0, - "volume_step": 0.1, - } - client.symbol_info_tick_as_dict.return_value = {"ask": 100.0, "bid": 99.0} - client.order_calc_margin.side_effect = [10.0, 20.0] - - result = calculate_margin_and_volume( - client, - "EURUSD", - unit_margin_ratio=0.0, - preserved_margin_ratio=0.0, - ) - - _assert_close(result["buy_volume"], 0.1) - _assert_close(result["sell_volume"], 0.0) - - def test_calculate_margin_and_volume_zero_ratio_preserves_margin_first( - self, - ) -> None: - """Test preserved margin reduces affordability before min sizing.""" - client = _mock_trade_client() - client.account_info_as_dict.return_value = {"margin_free": 100.0} - client.symbol_info_as_dict.return_value = { - "volume_min": 0.1, - "volume_max": 1.0, - "volume_step": 0.1, - } - client.symbol_info_tick_as_dict.return_value = {"ask": 100.0, "bid": 99.0} - client.order_calc_margin.side_effect = [11.0, 9.0] - - result = calculate_margin_and_volume( - client, - "EURUSD", - unit_margin_ratio=0.0, - preserved_margin_ratio=0.9, - ) - - _assert_close(result["available_margin"], 10.0) - _assert_close(result["buy_volume"], 0.0) - _assert_close(result["sell_volume"], 0.1) - def test_calculate_margin_and_volume_zero_ratio_without_available_margin( self, ) -> None: @@ -1762,42 +1572,41 @@ class TestVolumeAndExecution: _assert_close(calculate_projected_margin_ratio(client, symbol="EURUSD"), 0.05) - def test_projected_margin_ratio_adds_buy_exposure(self) -> None: - """Test projected buy margin is added to current symbol exposure.""" + @pytest.mark.parametrize( + ("side", "order_type", "price", "margin_return", "expected_ratio"), + [ + ("BUY", 10, 1.101, 25.0, 0.025), + ("SELL", 11, 1.1, 24.0, 0.024), + ], + ids=["buy", "sell"], + ) + def test_projected_margin_ratio_adds_side_exposure( + self, + side: OrderSide, + order_type: int, + price: float, + margin_return: float, + expected_ratio: float, + ) -> None: + """Test projected buy/sell margin uses ask/bid pricing and adds to exposure.""" client = _mock_trade_client() client.account_info_as_dict.return_value = {"equity": 1000.0} client.positions_get_as_df.return_value = pd.DataFrame() client.symbol_info_tick_as_dict.return_value = {"ask": 1.101, "bid": 1.1} - client.order_calc_margin.return_value = 25.0 + client.order_calc_margin.return_value = margin_return result = calculate_projected_margin_ratio( client, symbol="EURUSD", - new_position_side="BUY", + new_position_side=side, new_position_volume=0.1, ) - _assert_close(result, 0.025) - client.order_calc_margin.assert_called_once_with(10, "EURUSD", 0.1, 1.101) - - def test_projected_margin_ratio_adds_sell_exposure(self) -> None: - """Test projected sell margin uses bid pricing.""" - client = _mock_trade_client() - client.account_info_as_dict.return_value = {"equity": 1000.0} - client.positions_get_as_df.return_value = pd.DataFrame() - client.symbol_info_tick_as_dict.return_value = {"ask": 1.101, "bid": 1.1} - client.order_calc_margin.return_value = 24.0 - - result = calculate_projected_margin_ratio( - client, - symbol="EURUSD", - new_position_side="SELL", - new_position_volume=0.1, + _assert_close(result, expected_ratio) + client.order_calc_margin.assert_called_once_with( + order_type, "EURUSD", 0.1, price ) - _assert_close(result, 0.024) - client.order_calc_margin.assert_called_once_with(11, "EURUSD", 0.1, 1.1) - @pytest.mark.parametrize( ("account", "kwargs", "candidate_margin", "expected_ratio"), [ @@ -2013,12 +1822,21 @@ class TestVolumeAndExecution: with pytest.raises(Mt5OperationError, match="Account equity"): calculate_projected_margin_ratio(client, symbol="EURUSD") - def test_symbol_group_margin_ratio_suppresses_projected_failure( + @pytest.mark.parametrize( + ("suppress_errors", "expected_outcome"), + [ + pytest.param(True, "suppressed", id="suppresses-projected-failure"), + pytest.param(False, "raised", id="reraises-projected-failure"), + ], + ) + def test_symbol_group_margin_ratio_projected_failure( self, + suppress_errors: bool, + expected_outcome: str, mocker: MockerFixture, caplog: pytest.LogCaptureFixture, ) -> None: - """Test projected margin failures can be skipped for safe group reads.""" + """Test add-mode projected margin failures suppress or reraise.""" client = _mock_trade_client() client.account_info_as_dict.return_value = {"equity": 1000.0} mocker.patch( @@ -2030,35 +1848,22 @@ class TestVolumeAndExecution: side_effect=Mt5OperationError("bad tick"), ) - with caplog.at_level(logging.WARNING, logger="mt5cli.trading"): - result = calculate_symbol_group_margin_ratio( - client, - symbols=["EURUSD"], - new_symbol="EURUSD", - new_position_side="BUY", - new_position_volume=0.1, - suppress_errors=True, - ) - - _assert_close(result, 0.0) - assert "Skipping projected margin" in caplog.text - - def test_symbol_group_margin_ratio_reraises_projected_failure( - self, - mocker: MockerFixture, - ) -> None: - """Test projected margin failures raise when suppression is disabled.""" - client = _mock_trade_client() - client.account_info_as_dict.return_value = {"equity": 1000.0} - mocker.patch( - "mt5cli.trading.calculate_positions_margin_by_symbol", - return_value={}, - ) - mocker.patch( - "mt5cli.trading.estimate_order_margin", - side_effect=Mt5OperationError("bad tick"), - ) + if suppress_errors: + assert expected_outcome == "suppressed" + with caplog.at_level(logging.WARNING, logger="mt5cli.trading"): + result = calculate_symbol_group_margin_ratio( + client, + symbols=["EURUSD"], + new_symbol="EURUSD", + new_position_side="BUY", + new_position_volume=0.1, + suppress_errors=True, + ) + _assert_close(result, 0.0) + assert "Skipping projected margin" in caplog.text + return + assert expected_outcome == "raised" with pytest.raises(Mt5OperationError, match="bad tick"): calculate_symbol_group_margin_ratio( client, @@ -2166,12 +1971,21 @@ class TestVolumeAndExecution: _assert_close(result, 0.04) - def test_symbol_group_margin_ratio_replace_suppresses_candidate_failure( + @pytest.mark.parametrize( + ("suppress_errors", "expected_ratio"), + [ + pytest.param(True, 0.025, id="suppresses"), + pytest.param(False, None, id="reraises"), + ], + ) + def test_symbol_group_margin_ratio_replace_mode_candidate_failure( self, + suppress_errors: bool, + expected_ratio: float | None, mocker: MockerFixture, caplog: pytest.LogCaptureFixture, ) -> None: - """Test replace mode skips both subtract and add when estimation fails.""" + """Test replace-mode candidate failures suppress or reraise.""" client = _mock_trade_client() client.account_info_as_dict.return_value = {"equity": 1000.0} mocker.patch( @@ -2183,36 +1997,21 @@ class TestVolumeAndExecution: side_effect=Mt5OperationError("bad tick"), ) - with caplog.at_level(logging.WARNING, logger="mt5cli.trading"): - result = calculate_symbol_group_margin_ratio( - client, - symbols=["EURUSD"], - new_symbol="EURUSD", - new_position_side="BUY", - new_position_volume=0.1, - projection_mode="replace_symbol", - suppress_errors=True, - ) - - # When candidate fails, neither subtraction nor addition is applied - _assert_close(result, 0.025) - assert "Skipping projected margin" in caplog.text - - def test_symbol_group_margin_ratio_replace_reraises_candidate_failure( - self, - mocker: MockerFixture, - ) -> None: - """Test replace mode re-raises candidate failure when suppress_errors=False.""" - client = _mock_trade_client() - client.account_info_as_dict.return_value = {"equity": 1000.0} - mocker.patch( - "mt5cli.trading.calculate_positions_margin_by_symbol", - return_value={"EURUSD": 25.0}, - ) - mocker.patch( - "mt5cli.trading.estimate_order_margin", - side_effect=Mt5OperationError("bad tick"), - ) + if suppress_errors: + with caplog.at_level(logging.WARNING, logger="mt5cli.trading"): + result = calculate_symbol_group_margin_ratio( + client, + symbols=["EURUSD"], + new_symbol="EURUSD", + new_position_side="BUY", + new_position_volume=0.1, + projection_mode="replace_symbol", + suppress_errors=True, + ) + # When candidate fails, neither subtraction nor addition is applied + _assert_close(result, cast("float", expected_ratio)) + assert "Skipping projected margin" in caplog.text + return with pytest.raises(Mt5OperationError, match="bad tick"): calculate_symbol_group_margin_ratio( @@ -2302,33 +2101,30 @@ class TestVolumeAndExecution: _assert_close(_request_from_result(result)["sl"], 1.0) _assert_close(_request_from_result(result)["tp"], 1.4) - def test_place_market_order_rejects_invalid_filling_mode(self) -> None: - """Test MT5 filling mode names are validated before getattr.""" + @pytest.mark.parametrize( + ("mode_kwarg", "match"), + [ + ("order_filling_mode", "Unsupported order_filling mode"), + ("order_time_mode", "Unsupported order_time mode"), + ], + ids=["filling-mode", "time-mode"], + ) + def test_place_market_order_rejects_invalid_mode( + self, + mode_kwarg: str, + match: str, + ) -> None: + """Test MT5 filling and time mode names are validated before getattr.""" client = _mock_trade_client() client.symbol_info_tick_as_dict.return_value = {"ask": 1.2, "bid": 1.1} - with pytest.raises(ValueError, match="Unsupported order_filling mode"): + with pytest.raises(ValueError, match=match): place_market_order( client, symbol="EURUSD", volume=0.1, order_side="BUY", - order_filling_mode=cast("Any", "BAD"), - dry_run=True, - ) - - def test_place_market_order_rejects_invalid_time_mode(self) -> None: - """Test MT5 time mode names are validated before getattr.""" - client = _mock_trade_client() - client.symbol_info_tick_as_dict.return_value = {"ask": 1.2, "bid": 1.1} - - with pytest.raises(ValueError, match="Unsupported order_time mode"): - place_market_order( - client, - symbol="EURUSD", - volume=0.1, - order_side="BUY", - order_time_mode=cast("Any", "BAD"), + **{mode_kwarg: cast("Any", "BAD")}, dry_run=True, ) @@ -2455,7 +2251,29 @@ class TestVolumeAndExecution: assert result["retcode"] == expected_retcode assert result["status"] == "failed" - def test_close_open_positions_filters_and_dry_runs(self) -> None: + @pytest.mark.parametrize( + ("filter_kwargs", "expected_order_side", "expected_position"), + [ + pytest.param( + {"symbols": "EURUSD"}, + "SELL", + 1, + id="filter-by-symbol", + ), + pytest.param( + {"tickets": [2]}, + "BUY", + 2, + id="filter-by-ticket", + ), + ], + ) + def test_close_open_positions_filters_and_dry_runs( + self, + filter_kwargs: dict[str, object], + expected_order_side: str, + expected_position: int, + ) -> None: """Test close helper filters positions and builds opposite orders.""" client = _mock_trade_client() client.positions_get_as_df.return_value = pd.DataFrame( @@ -2466,27 +2284,13 @@ class TestVolumeAndExecution: ) client.symbol_info_tick_as_dict.return_value = {"ask": 1.2, "bid": 1.1} - result = close_open_positions(client, symbols="EURUSD", dry_run=True) - - assert len(result) == 1 - assert result[0]["order_side"] == "SELL" - assert _request_from_result(result[0])["position"] == 1 - - def test_close_open_positions_filters_by_ticket(self) -> None: - """Test close helper can filter by ticket.""" - client = _mock_trade_client() - client.positions_get_as_df.return_value = pd.DataFrame( - [ - {"ticket": 1, "symbol": "EURUSD", "type": 0, "volume": 0.1}, - {"ticket": 2, "symbol": "USDJPY", "type": 1, "volume": 0.2}, - ], + result = close_open_positions( + client, dry_run=True, **cast("Any", filter_kwargs) ) - client.symbol_info_tick_as_dict.return_value = {"ask": 1.2, "bid": 1.1} - - result = close_open_positions(client, tickets=[2], dry_run=True) assert len(result) == 1 - assert result[0]["order_side"] == "BUY" + assert result[0]["order_side"] == expected_order_side + assert _request_from_result(result[0])["position"] == expected_position def test_close_open_positions_sends_position_ticket(self) -> None: """Test live close orders include the position before order_send.""" @@ -2515,41 +2319,63 @@ class TestVolumeAndExecution: == {} ) - def test_calculate_trailing_stop_updates_buy_positions(self) -> None: - """Test buy trailing stops use bid and improve only upward.""" - client = _mock_trade_client() - client.positions_get_as_df.return_value = pd.DataFrame( - [ - {"ticket": 1, "symbol": "EURUSD", "type": 0, "volume": 0.1, "sl": 1.0}, - { - "ticket": 2, - "symbol": "EURUSD", - "type": 0, - "volume": 0.1, - "sl": 1.19, - }, - ], - ) - client.symbol_info_tick_as_dict.return_value = {"bid": 1.2, "ask": 1.201} - client.symbol_info_as_dict.return_value = {"digits": 4} - - result = calculate_trailing_stop_updates( - client, - symbol="EURUSD", - trailing_stop_ratio=0.01, - ) - - assert result == {1: 1.188} - - def test_calculate_trailing_stop_updates_buy_positions_ignore_invalid_ask( + @pytest.mark.parametrize( + ("positions", "tick", "expected"), + [ + pytest.param( + [ + { + "ticket": 1, + "symbol": "EURUSD", + "type": 0, + "volume": 0.1, + "sl": 1.0, + }, + { + "ticket": 2, + "symbol": "EURUSD", + "type": 0, + "volume": 0.1, + "sl": 1.19, + }, + ], + {"bid": 1.2, "ask": 1.201}, + {1: 1.188}, + id="buy-uses-bid-skips-already-favorable", + ), + pytest.param( + [ + { + "ticket": 3, + "symbol": "EURUSD", + "type": 1, + "volume": 0.1, + "sl": 1.3, + }, + { + "ticket": 4, + "symbol": "EURUSD", + "type": 1, + "volume": 0.1, + "sl": 1.21, + }, + ], + {"bid": 1.198, "ask": 1.2}, + {3: 1.212}, + id="sell-uses-ask-skips-already-favorable", + ), + ], + ) + def test_calculate_trailing_stop_updates_by_side( self, + positions: list[dict[str, object]], + tick: dict[str, float], + expected: dict[int, float], ) -> None: - """Test buy trailing stops do not require an ask price.""" + """Buy uses bid (improves up); sell uses ask (improves down).""" client = _mock_trade_client() - client.positions_get_as_df.return_value = pd.DataFrame( - [{"ticket": 1, "symbol": "EURUSD", "type": 0, "volume": 0.1, "sl": 1.0}], - ) - client.symbol_info_tick_as_dict.return_value = {"bid": 1.2, "ask": 0.0} + client.positions_get_as_df.return_value = pd.DataFrame(positions) + client.symbol_info_tick_as_dict.return_value = tick client.symbol_info_as_dict.return_value = {"digits": 4} result = calculate_trailing_stop_updates( @@ -2558,43 +2384,51 @@ class TestVolumeAndExecution: trailing_stop_ratio=0.01, ) - assert result == {1: 1.188} + assert result == expected - def test_calculate_trailing_stop_updates_sell_positions(self) -> None: - """Test sell trailing stops use ask and improve only downward.""" - client = _mock_trade_client() - client.positions_get_as_df.return_value = pd.DataFrame( - [ - {"ticket": 3, "symbol": "EURUSD", "type": 1, "volume": 0.1, "sl": 1.3}, - { - "ticket": 4, - "symbol": "EURUSD", - "type": 1, - "volume": 0.1, - "sl": 1.21, - }, - ], - ) - client.symbol_info_tick_as_dict.return_value = {"bid": 1.198, "ask": 1.2} - client.symbol_info_as_dict.return_value = {"digits": 4} - - result = calculate_trailing_stop_updates( - client, - symbol="EURUSD", - trailing_stop_ratio=0.01, - ) - - assert result == {3: 1.212} - - def test_calculate_trailing_stop_updates_sell_positions_ignore_invalid_bid( + @pytest.mark.parametrize( + ("positions", "tick", "expected"), + [ + pytest.param( + [ + { + "ticket": 1, + "symbol": "EURUSD", + "type": 0, + "volume": 0.1, + "sl": 1.0, + }, + ], + {"bid": 1.2, "ask": 0.0}, + {1: 1.188}, + id="buy-ignores-invalid-ask", + ), + pytest.param( + [ + { + "ticket": 3, + "symbol": "EURUSD", + "type": 1, + "volume": 0.1, + "sl": 1.3, + }, + ], + {"bid": 0.0, "ask": 1.2}, + {3: 1.212}, + id="sell-ignores-invalid-bid", + ), + ], + ) + def test_calculate_trailing_stop_updates_ignores_opposite_side_price( self, + positions: list[dict[str, object]], + tick: dict[str, object], + expected: dict[int, float], ) -> None: - """Test sell trailing stops do not require a bid price.""" + """Buy uses bid only; sell uses ask only (other-side price may be invalid).""" client = _mock_trade_client() - client.positions_get_as_df.return_value = pd.DataFrame( - [{"ticket": 3, "symbol": "EURUSD", "type": 1, "volume": 0.1, "sl": 1.3}], - ) - client.symbol_info_tick_as_dict.return_value = {"bid": 0.0, "ask": 1.2} + client.positions_get_as_df.return_value = pd.DataFrame(positions) + client.symbol_info_tick_as_dict.return_value = tick client.symbol_info_as_dict.return_value = {"digits": 4} result = calculate_trailing_stop_updates( @@ -2603,7 +2437,7 @@ class TestVolumeAndExecution: trailing_stop_ratio=0.01, ) - assert result == {3: 1.212} + assert result == expected def test_calculate_trailing_stop_updates_invalid_bid_or_ask(self) -> None: """Test invalid side-specific tick prices fail safely without updates.""" @@ -2627,8 +2461,25 @@ class TestVolumeAndExecution: == {} ) + @pytest.mark.parametrize( + ("tick", "expected"), + [ + pytest.param( + {"bid": 1.2, "ask": 0.0}, + {1: 1.188}, + id="invalid-ask-skips-sell-updates", + ), + pytest.param( + {"bid": None, "ask": 1.2}, + {2: 1.212}, + id="invalid-bid-skips-buy-updates", + ), + ], + ) def test_calculate_trailing_stop_updates_mixed_positions_skip_invalid_side( self, + tick: dict[str, object], + expected: dict[int, float], ) -> None: """Test one invalid side price does not block the valid side.""" client = _mock_trade_client() @@ -2639,20 +2490,15 @@ class TestVolumeAndExecution: ], ) client.symbol_info_as_dict.return_value = {"digits": 4} + client.symbol_info_tick_as_dict.return_value = tick - client.symbol_info_tick_as_dict.return_value = {"bid": 1.2, "ask": 0.0} - assert calculate_trailing_stop_updates( + result = calculate_trailing_stop_updates( client, symbol="EURUSD", trailing_stop_ratio=0.01, - ) == {1: 1.188} + ) - client.symbol_info_tick_as_dict.return_value = {"bid": None, "ask": 1.2} - assert calculate_trailing_stop_updates( - client, - symbol="EURUSD", - trailing_stop_ratio=0.01, - ) == {2: 1.212} + assert result == expected def test_calculate_trailing_stop_updates_invalid_symbol_digits(self) -> None: """Test invalid symbol metadata fails safely without updates.""" diff --git a/tests/test_utils.py b/tests/test_utils.py index 415335a..30916f4 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -89,28 +89,45 @@ class TestExportDataframe: """Create a sample DataFrame for testing.""" return pd.DataFrame({"a": [1, 2, 3], "b": ["x", "y", "z"]}) - def test_export_csv(self, tmp_path: Path, sample_df: pd.DataFrame) -> None: - """Test CSV export.""" - output = tmp_path / "out.csv" - export_dataframe(sample_df, output, "csv") - result = pd.read_csv(output) - pd.testing.assert_frame_equal(result, sample_df) - - def test_export_json(self, tmp_path: Path, sample_df: pd.DataFrame) -> None: - """Test JSON export.""" - output = tmp_path / "out.json" - export_dataframe(sample_df, output, "json") - with output.open() as f: - records = json.load(f) - assert len(records) == 3 - assert records[0]["a"] == 1 - - def test_export_parquet(self, tmp_path: Path, sample_df: pd.DataFrame) -> None: - """Test Parquet export.""" - output = tmp_path / "out.parquet" - export_dataframe(sample_df, output, "parquet") - result = pd.read_parquet(output) - pd.testing.assert_frame_equal(result, sample_df) + @pytest.mark.parametrize( + ("filename", "output_format", "reader"), + [ + ("out.csv", "csv", "csv"), + ("out.json", "json", "json"), + ("out.parquet", "parquet", "parquet"), + ("out.db", "sqlite3", "sqlite3"), + ], + ids=["csv", "json", "parquet", "sqlite3"], + ) + def test_export_round_trip( + self, + tmp_path: Path, + sample_df: pd.DataFrame, + filename: str, + output_format: str, + reader: str, + ) -> None: + """Test CSV/JSON/Parquet/SQLite3 exports round-trip the sample DataFrame.""" + output = tmp_path / filename + export_dataframe(sample_df, output, output_format, table_name="test_table") + if reader == "csv": + result = pd.read_csv(output) + pd.testing.assert_frame_equal(result, sample_df) + elif reader == "json": + with output.open() as f: + records = json.load(f) + assert len(records) == 3 + assert records[0]["a"] == 1 + elif reader == "sqlite3": + with sqlite3.connect(output) as conn: + result = pd.read_sql( # type: ignore[reportUnknownMemberType] + "SELECT * FROM test_table", + conn, + ) + pd.testing.assert_frame_equal(result, sample_df) + else: + result = pd.read_parquet(output) + pd.testing.assert_frame_equal(result, sample_df) def test_export_parquet_without_pyarrow( self, @@ -123,17 +140,6 @@ class TestExportDataframe: with pytest.raises(ImportError, match="mt5cli\\[parquet\\]"): export_dataframe(sample_df, tmp_path / "out.parquet", "parquet") - def test_export_sqlite3(self, tmp_path: Path, sample_df: pd.DataFrame) -> None: - """Test SQLite3 export.""" - output = tmp_path / "out.db" - export_dataframe(sample_df, output, "sqlite3", table_name="test_table") - with sqlite3.connect(output) as conn: - result = pd.read_sql( # type: ignore[reportUnknownMemberType] - "SELECT * FROM test_table", - conn, - ) - pd.testing.assert_frame_equal(result, sample_df) - def test_unsupported_format_raises( self, tmp_path: Path, @@ -147,13 +153,41 @@ class TestExportDataframe: class TestExportDataframeToSqlite: """Tests for export_dataframe_to_sqlite.""" - def test_append_preserves_existing_rows(self, tmp_path: Path) -> None: - """Test append mode keeps prior rows in the SQLite table.""" + @pytest.mark.parametrize( + ("first_if_exists", "second_if_exists"), + [ + pytest.param(IfExists.REPLACE, IfExists.APPEND, id="replace-then-append"), + pytest.param(None, None, id="default-append"), + ], + ) + def test_append_preserves_existing_rows( + self, + tmp_path: Path, + first_if_exists: IfExists | None, + second_if_exists: IfExists | None, + ) -> None: + """Test explicit append and default append modes keep prior rows.""" output = tmp_path / "append.db" first = pd.DataFrame({"id": [1], "value": ["a"]}) second = pd.DataFrame({"id": [2], "value": ["b"]}) - export_dataframe_to_sqlite(first, output, "items", if_exists=IfExists.REPLACE) - export_dataframe_to_sqlite(second, output, "items", if_exists=IfExists.APPEND) + if first_if_exists is None: + export_dataframe_to_sqlite(first, output, "items") + else: + export_dataframe_to_sqlite( + first, + output, + "items", + if_exists=first_if_exists, + ) + if second_if_exists is None: + export_dataframe_to_sqlite(second, output, "items") + else: + export_dataframe_to_sqlite( + second, + output, + "items", + if_exists=second_if_exists, + ) with sqlite3.connect(output) as conn: result = pd.read_sql( # type: ignore[reportUnknownMemberType] "SELECT id, value FROM items ORDER BY id", @@ -205,26 +239,6 @@ class TestExportDataframeToSqlite: }), ) - def test_default_if_exists_appends_without_dropping_rows( - self, - tmp_path: Path, - ) -> None: - """Test the default append mode keeps prior rows.""" - output = tmp_path / "default-append.db" - first = pd.DataFrame({"id": [1], "value": ["a"]}) - second = pd.DataFrame({"id": [2], "value": ["b"]}) - export_dataframe_to_sqlite(first, output, "items") - export_dataframe_to_sqlite(second, output, "items") - with sqlite3.connect(output) as conn: - result = pd.read_sql( # type: ignore[reportUnknownMemberType] - "SELECT id, value FROM items ORDER BY id", - conn, - ) - pd.testing.assert_frame_equal( - result, - pd.DataFrame({"id": [1, 2], "value": ["a", "b"]}), - ) - def test_writes_index_with_label(self, tmp_path: Path) -> None: """Test optional index export with a custom label.""" output = tmp_path / "index.db" @@ -258,15 +272,17 @@ class TestExportDataframeToSqlite: class TestParseDatetime: """Tests for parse_datetime.""" - def test_valid_date(self) -> None: - """Test parsing a date string.""" - result = parse_datetime("2024-01-15") - assert result == datetime(2024, 1, 15, tzinfo=UTC) - - def test_valid_datetime_with_tz(self) -> None: - """Test parsing a datetime with timezone.""" - result = parse_datetime("2024-01-15T12:00:00+00:00") - assert result == datetime(2024, 1, 15, 12, 0, 0, tzinfo=UTC) + @pytest.mark.parametrize( + ("value", "expected"), + [ + ("2024-01-15", datetime(2024, 1, 15, tzinfo=UTC)), + ("2024-01-15T12:00:00+00:00", datetime(2024, 1, 15, 12, 0, 0, tzinfo=UTC)), + ], + ids=["date", "datetime-with-tz"], + ) + def test_valid_inputs(self, value: str, expected: datetime) -> None: + """Test parsing valid date and datetime strings.""" + assert parse_datetime(value) == expected def test_invalid_format_raises(self) -> None: """Test that invalid format raises ValueError.""" @@ -279,26 +295,29 @@ class TestParseTimeframe: @pytest.mark.parametrize( ("value", "expected"), - [("M1", 1), ("h1", 16385), ("D1", 16408), ("MN1", 49153)], + [ + ("M1", 1), + ("h1", 16385), + ("D1", 16408), + ("MN1", 49153), + ("1", 1), + (16385, 16385), + ], + ids=["M1", "h1", "D1", "MN1", "int-string-1", "int-16385"], ) - def test_named_timeframe(self, value: str, expected: int) -> None: - """Test parsing named timeframes.""" + def test_valid_timeframe(self, value: str | int, expected: int) -> None: + """Test parsing valid string and integer timeframe values.""" assert parse_timeframe(value) == expected - def test_integer_timeframe(self) -> None: - """Test parsing supported integer timeframes.""" - assert parse_timeframe("1") == 1 - assert parse_timeframe(16385) == 16385 - - def test_unsupported_integer_timeframe_raises(self) -> None: - """Test that unsupported integer timeframes raise ValueError.""" + @pytest.mark.parametrize( + "value", + ["42", "INVALID"], + ids=["unsupported-integer", "invalid-string"], + ) + def test_invalid_timeframe_raises(self, value: str) -> None: + """Test that invalid timeframe values raise ValueError.""" with pytest.raises(ValueError, match="Invalid timeframe"): - parse_timeframe("42") - - def test_invalid_timeframe_raises(self) -> None: - """Test that invalid timeframe raises ValueError.""" - with pytest.raises(ValueError, match="Invalid timeframe"): - parse_timeframe("INVALID") + parse_timeframe(value) class TestParseTickFlags: @@ -306,26 +325,29 @@ class TestParseTickFlags: @pytest.mark.parametrize( ("value", "expected"), - [("ALL", -1), ("info", 1), ("TRADE", 2), ("COPY_TICKS_ALL", -1)], + [ + ("ALL", -1), + ("info", 1), + ("TRADE", 2), + ("COPY_TICKS_ALL", -1), + ("-1", -1), + (2, 2), + ], + ids=["ALL", "info", "TRADE", "COPY_TICKS_ALL", "int-string--1", "int-2"], ) - def test_named_flag(self, value: str, expected: int) -> None: - """Test parsing named tick flags.""" + def test_valid_flag(self, value: str | int, expected: int) -> None: + """Test parsing valid string and integer tick flag values.""" assert parse_tick_flags(value) == expected - def test_integer_flag(self) -> None: - """Test parsing supported integer tick flags.""" - assert parse_tick_flags("-1") == -1 - assert parse_tick_flags(2) == 2 - - def test_unsupported_integer_flag_raises(self) -> None: - """Test that unsupported integer tick flags raise ValueError.""" + @pytest.mark.parametrize( + "value", + ["7", "INVALID"], + ids=["unsupported-integer", "invalid-string"], + ) + def test_invalid_flag_raises(self, value: str) -> None: + """Test that invalid tick flag values raise ValueError.""" with pytest.raises(ValueError, match="Invalid tick flags"): - parse_tick_flags("7") - - def test_invalid_flag_raises(self) -> None: - """Test that invalid flag raises ValueError.""" - with pytest.raises(ValueError, match="Invalid tick flags"): - parse_tick_flags("INVALID") + parse_tick_flags(value) # --------------------------------------------------------------------------- @@ -348,15 +370,17 @@ class TestParseRequest: result = parse_request(f"@{path}") assert result == {"action": 2} - def test_invalid_json_raises(self) -> None: - """Test that invalid JSON raises ValueError.""" - with pytest.raises(ValueError, match="Invalid JSON request"): - parse_request("not json") - - def test_non_object_raises(self) -> None: - """Test that a non-object JSON raises ValueError.""" - with pytest.raises(ValueError, match="must be a JSON object"): - parse_request("[1, 2, 3]") + @pytest.mark.parametrize( + ("value", "match"), + [ + pytest.param("not json", "Invalid JSON request", id="invalid-json"), + pytest.param("[1, 2, 3]", "must be a JSON object", id="non-object"), + ], + ) + def test_invalid_request_raises(self, value: str, match: str) -> None: + """Test that invalid JSON and non-object requests raise ValueError.""" + with pytest.raises(ValueError, match=match): + parse_request(value) def test_missing_file_raises(self, tmp_path: Path) -> None: """Test that a missing request file raises ValueError.""" @@ -373,13 +397,10 @@ class TestParseRequest: class TestConstants: """Tests for module constants.""" - def test_timeframe_map_is_private_in_utils(self) -> None: - """TIMEFRAME_MAP is a private implementation detail; not a public attribute.""" - assert not hasattr(mt5cli.utils, "TIMEFRAME_MAP") - - def test_tick_flag_map_absent_from_utils(self) -> None: - """TICK_FLAG_MAP is not exposed by mt5cli.utils.""" - assert not hasattr(mt5cli.utils, "TICK_FLAG_MAP") + @pytest.mark.parametrize("name", ["TIMEFRAME_MAP", "TICK_FLAG_MAP"]) + def test_private_maps_absent_from_utils(self, name: str) -> None: + """Private pdmt5 maps are not exposed as public mt5cli.utils attributes.""" + assert not hasattr(mt5cli.utils, name) @pytest.mark.parametrize( ("dataset", "expected"), @@ -422,23 +443,24 @@ class TestDateTimeType: class TestTimeframeType: """Tests for _TimeframeType.""" - def test_convert_string(self) -> None: - """Test converting a string to timeframe integer.""" - assert TIMEFRAME_TYPE.convert("H1", None, None) == 16385 + @pytest.mark.parametrize( + ("value", "expected"), + [("H1", 16385), (16385, 16385)], + ids=["string", "int"], + ) + def test_convert_valid(self, value: str | int, expected: int) -> None: + """Test converting valid string and integer timeframe values.""" + assert TIMEFRAME_TYPE.convert(value, None, None) == expected - def test_convert_int(self) -> None: - """Test converting supported integer timeframe values.""" - assert TIMEFRAME_TYPE.convert(16385, None, None) == 16385 - - def test_convert_unsupported_int(self) -> None: - """Test that unsupported integer values raise BadParameter.""" + @pytest.mark.parametrize( + "value", + [42, "bad"], + ids=["unsupported-int", "invalid-string"], + ) + def test_convert_invalid(self, value: object) -> None: + """Test that unsupported int and invalid string values raise BadParameter.""" with pytest.raises(Exception, match="Invalid timeframe"): - TIMEFRAME_TYPE.convert(42, None, None) - - def test_convert_invalid(self) -> None: - """Test that invalid values raise BadParameter.""" - with pytest.raises(Exception, match="Invalid timeframe"): - TIMEFRAME_TYPE.convert("bad", None, None) + TIMEFRAME_TYPE.convert(value, None, None) @pytest.mark.parametrize("value", [True, False, None, 1.5]) def test_convert_invalid_types(self, value: object) -> None: @@ -450,18 +472,24 @@ class TestTimeframeType: class TestTickFlagsType: """Tests for _TickFlagsType.""" - def test_convert_string(self) -> None: - """Test converting a string to tick flags integer.""" - assert TICK_FLAGS_TYPE.convert("ALL", None, None) == -1 + @pytest.mark.parametrize( + ("value", "expected"), + [("ALL", -1), (2, 2)], + ids=["string", "int"], + ) + def test_convert_valid(self, value: str | int, expected: int) -> None: + """Test converting valid string and integer tick flag values.""" + assert TICK_FLAGS_TYPE.convert(value, None, None) == expected - def test_convert_int(self) -> None: - """Test converting supported integer tick flag values.""" - assert TICK_FLAGS_TYPE.convert(2, None, None) == 2 - - def test_convert_unsupported_int(self) -> None: - """Test that unsupported integer values raise BadParameter.""" + @pytest.mark.parametrize( + "value", + [7, "bad"], + ids=["unsupported-int", "invalid-string"], + ) + def test_convert_invalid(self, value: object) -> None: + """Test that unsupported int and invalid string values raise BadParameter.""" with pytest.raises(Exception, match="Invalid tick flags"): - TICK_FLAGS_TYPE.convert(7, None, None) + TICK_FLAGS_TYPE.convert(value, None, None) @pytest.mark.parametrize("value", [True, False, None, 1.5]) def test_convert_invalid_types(self, value: object) -> None: @@ -469,11 +497,6 @@ class TestTickFlagsType: with pytest.raises(Exception, match="Invalid tick flags"): TICK_FLAGS_TYPE.convert(value, None, None) - def test_convert_invalid(self) -> None: - """Test that invalid values raise BadParameter.""" - with pytest.raises(Exception, match="Invalid tick flags"): - TICK_FLAGS_TYPE.convert("bad", None, None) - class TestRequestType: """Tests for _RequestType."""