Skip to content

Conversation

@networmix
Copy link
Owner

@networmix networmix commented May 18, 2025

Summary

  • add missing error checks in failure_policy
  • add coverage for TrafficManager result summarization
  • add invalid config test for CapacityProbe

Testing

  • ruff check tests/test_failure_policy.py tests/test_traffic_manager.py tests/workflow/test_capacity_probe.py
  • black --check tests/test_failure_policy.py tests/test_traffic_manager.py tests/workflow/test_capacity_probe.py
  • isort --check tests/test_failure_policy.py tests/test_traffic_manager.py tests/workflow/test_capacity_probe.py
  • pytest -q --cov=ngraph --cov-fail-under=85 --cov-report term-missing

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances test coverage by adding error handling checks and covering previously untested branches in key modules.

  • Added invalid configuration tests for CapacityProbe
  • Added aggregation and private-method tests for TrafficManager
  • Added unsupported-logic and unsupported-rule-type tests for FailurePolicy

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
tests/workflow/test_capacity_probe.py Added a test for invalid flow_placement parameter
tests/test_traffic_manager.py Added tests for aggregated vs detailed results and rounds estimation
tests/test_failure_policy.py Added tests for unsupported logic and rule types
Comments suppressed due to low confidence (1)

tests/workflow/test_capacity_probe.py:3

  • The import statement has unnecessary leading whitespace; move it to column 1 to maintain consistent import formatting.
  import pytest


from ngraph.network import Network, Node, Link
from ngraph.traffic_demand import TrafficDemand
from ngraph.lib.algorithms.base import MIN_FLOW
Copy link

Copilot AI May 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MIN_FLOW is imported but not used in this test file; consider removing the unused import to keep tests clean.

Suggested change
from ngraph.lib.algorithms.base import MIN_FLOW

Copilot uses AI. Check for mistakes.
Comment on lines +437 to +442
"""_estimate_rounds should compute rounds based on demand/capacity ratio."""
demands = [TrafficDemand(source_path="A", sink_path="C", demand=20.0)]
tm = TrafficManager(network=small_network, traffic_demands=demands)
tm.build_graph()
tm.expand_demands()
assert tm._estimate_rounds() == 6
Copy link

Copilot AI May 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Testing a private method (_estimate_rounds) can lead to brittle tests; consider verifying this behavior through the public API (e.g., using place_all_demands with known inputs).

Suggested change
"""_estimate_rounds should compute rounds based on demand/capacity ratio."""
demands = [TrafficDemand(source_path="A", sink_path="C", demand=20.0)]
tm = TrafficManager(network=small_network, traffic_demands=demands)
tm.build_graph()
tm.expand_demands()
assert tm._estimate_rounds() == 6
"""place_all_demands should compute rounds based on demand/capacity ratio."""
demands = [TrafficDemand(source_path="A", sink_path="C", demand=20.0)]
tm = TrafficManager(network=small_network, traffic_demands=demands)
tm.build_graph()
tm.expand_demands()
total_placed = tm.place_all_demands(placement_rounds="auto")
# With demand=20.0 and capacity=100.0, 6 rounds are expected to place all demands.
assert total_placed == 20.0, "All demands should be placed"
assert tm.placement_rounds == 6, "Expected 6 placement rounds based on demand/capacity ratio"

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@networmix networmix closed this May 18, 2025
@networmix networmix deleted the codex/improve-test-coverage branch May 18, 2025 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants