Skip to content

Refine part implementation#16

Open
RandVid wants to merge 3 commits intomainfrom
refine_part_implementation
Open

Refine part implementation#16
RandVid wants to merge 3 commits intomainfrom
refine_part_implementation

Conversation

@RandVid
Copy link
Copy Markdown
Collaborator

@RandVid RandVid commented Sep 18, 2025

No description provided.

Copy link
Copy Markdown
Owner

@aircode610 aircode610 left a comment

Choose a reason for hiding this comment

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

🤖 Test Coverage Review — see inline comments below.

Copy link
Copy Markdown
Owner

@aircode610 aircode610 left a comment

Choose a reason for hiding this comment

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

Test coverage review — 1 uncovered symbol found. See inline comment.

for node, node in to_remove:
self.bad_graph.pop(node, None)

def get_some_cycle_from_scc(self, scc: List[str]) -> List[str]:
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[Tests] Missing test for get_some_cycle_from_scc

get_some_cycle_from_scc was added but no corresponding test was found. There is no test file for conflict_resolving.py anywhere in the repository.

Suggested test cases:

  1. Happy path — simple cycle: Build a graph with a 3-node SCC (A→B→C→A). Call get_some_cycle_from_scc(["A","B","C"]) and assert the returned list forms a valid cycle (each node leads to the next in good_graph_1).

  2. Single-node SCC with self-loop: Create a graph where node "X" has an edge to itself. Pass scc=["X"] and assert the result is ["X"].

  3. Single-node SCC without self-loop: Create a graph where node "X" has no self-loop. Pass scc=["X"] and assert the result is [] (empty list — no cycle found).

  4. Edge case — larger SCC with multiple possible cycles: Build a graph with 5 nodes and several edges forming multiple cycles. Assert the returned list is non-empty and forms a valid cycle within the SCC.

  5. Error case — empty SCC list: Pass scc=[] and verify the behavior (currently dfs(scc[0]) would raise IndexError). Consider whether this should be handled gracefully or documented as a precondition.

@aircode610
Copy link
Copy Markdown
Owner

🤖 Test Coverage Review

Summary
1 function added without corresponding tests.

Missing Tests

  • conflict_resolving.py:268get_some_cycle_from_scc() → see inline comment

Notes

  • No tests/ directory or test files exist in the repository. Consider adding a test infrastructure (e.g., tests/test_conflict_resolving.py with pytest).
  • streamlit_app.pyprose_tab() was modified (body changes only, no new function definitions added in the diff). This is UI/Streamlit code; while integration testing would be beneficial, no new standalone-testable functions were introduced.
  • The inner dfs() helper inside get_some_cycle_from_scc is a nested private function and was intentionally not flagged — it is tested indirectly via get_some_cycle_from_scc.

What looks good

  • The get_some_cycle_from_scc method has thorough docstring documentation covering args, return values, and algorithm description.
  • The integration with resolve_cycles_and_conflicts (lines 409, 422) correctly passes the SCC through the new method before handing the cycle to resolve_cycle_by_user.

Review by automated test-coverage agent. Advisory only — does not approve or request changes.

@aircode610
Copy link
Copy Markdown
Owner

🤖 Test Coverage Review

Summary
1 function added or modified without corresponding tests (already flagged in a prior review).

Missing Tests

  • conflict_resolving.py:268get_some_cycle_from_scc() → existing inline comment already posted (not duplicated)

Notes

  • streamlit_app.py — The prose_tab() function body was significantly modified (85 lines added), but the function definition line itself was not newly added in this diff. The new prose-generation logic (LLM invocation, prompt construction, Streamlit UI wiring) inside prose_tab() would benefit from unit tests, but since no new standalone function or class was introduced, no inline comment was posted.
  • dfs() is a nested helper inside get_some_cycle_from_scc and is implicitly covered by testing the outer function.
  • No test files exist anywhere in this repository. Consider adding a tests/ directory with at least tests/test_conflict_resolving.py and tests/test_streamlit_app.py.

Recommendations

  1. Add tests for get_some_cycle_from_scc as described in the existing inline review comment.
  2. Consider extracting the prompt-building logic in prose_tab() (the instruction string construction and prompt formatting) into a standalone, testable function — this would make the LLM integration easier to test without requiring a running Streamlit session.
  3. Set up a basic test infrastructure (pytest, tests/ directory, CI integration) for the project.

Review by automated agent. Advisory only.

@aircode610
Copy link
Copy Markdown
Owner

🤖 Test Coverage Review

Summary
1 function added or modified without corresponding tests. An inline comment already exists for it from a prior review.

Missing Tests

  • conflict_resolving.py:268get_some_cycle_from_scc() → inline comment already posted (not duplicated)

Details

  • get_some_cycle_from_scc is a new public method added to conflict_resolving.py. There are no test files anywhere in this repository (tests/ directory does not exist), so this method has zero test coverage. A prior review already left a detailed inline comment with 5 suggested test cases — that comment still applies.
  • streamlit_app.py — the prose_tab() function body was modified (UI logic for prose generation), but no new function or class definitions were introduced on added lines. No inline comment required.
  • The nested dfs() helper inside get_some_cycle_from_scc is a private inner function and was intentionally skipped.

Recommendation
Consider adding a tests/ directory with at least a tests/test_conflict_resolving.py file covering the get_some_cycle_from_scc method (and ideally other public methods in ConflictResolver).


Review by automated agent. Advisory only.

@aircode610
Copy link
Copy Markdown
Owner

🤖 Test Coverage Review

Summary
1 function added or modified without corresponding tests.

Missing Tests

  • conflict_resolving.py:268get_some_cycle_from_scc() → inline comment already exists from a previous review (not duplicated)

Details

  • get_some_cycle_from_scc is a new public method added to the conflict-resolving graph class. There is no tests/ directory in this repository and no test files exist anywhere for conflict_resolving.py. The existing inline review comment on this symbol (posted previously) already contains suggested test cases.
  • streamlit_app.py — the prose_tab() function body was substantially modified (new LLM-based prose generation logic), but the function definition line itself was not added in this diff. No new function or class definitions were introduced in this file.
  • The nested dfs() helper inside get_some_cycle_from_scc is an inner function and not independently testable.

Recommendation
Consider creating a tests/ directory with at minimum tests/test_conflict_resolving.py covering the get_some_cycle_from_scc method. See the existing inline comment for 5 concrete test case suggestions.


Review by automated agent. Advisory only.

@aircode610
Copy link
Copy Markdown
Owner

🤖 Test Coverage Review

Summary
1 function added or modified without corresponding tests. A prior review has already flagged it — no additional inline comments are needed.

Prior Review Coverage

  • conflict_resolving.py:268get_some_cycle_from_scc() → already flagged in an existing inline comment with 5 concrete test case suggestions. No duplicate comment posted.

Skipped (UI-only / not independently unit-testable)

  • streamlit_app.pyprose_tab() changes are Streamlit UI wiring (text areas, buttons, inline LLM calls). No separable business logic to unit-test.

⚠️ No Test Infrastructure
This repository has no tests/ directory, no conftest.py, and no test files anywhere. Before adding tests for get_some_cycle_from_scc, I recommend setting up test infrastructure:

  1. Create a tests/ directory with an empty __init__.py.
  2. Add conftest.py with shared fixtures (e.g., a helper to construct a ConflictResolver instance with a known graph).
  3. Add pytest configuration — either in pyproject.toml:
    [tool.pytest.ini_options]
    testpaths = ["tests"]
    or in a setup.cfg / pytest.ini.
  4. Create tests/test_conflict_resolving.py and implement the 5 test cases already suggested in the prior inline comment:
    • Happy path: 3-node SCC forming a simple cycle
    • Single-node SCC with self-loop → returns ["X"]
    • Single-node SCC without self-loop → returns []
    • Larger SCC with multiple possible cycles → non-empty valid cycle
    • Empty SCC list → document or handle IndexError on scc[0]

Review by automated test-coverage agent. Advisory only — no approval or change request.

@aircode610
Copy link
Copy Markdown
Owner

🤖 Test Coverage Review

Summary: 1 function added or modified without corresponding tests.

Missing Tests

File Line Symbol Note
conflict_resolving.py 268 get_some_cycle_from_scc() ⚠️ Inline comment already posted in a prior review — see existing comment

Skipped (not independently testable)

  • streamlit_app.py — all changes are inside prose_tab(), a Streamlit UI callback. Testing would require mocking Streamlit internals; no new standalone/pure-logic functions were introduced.
  • dfs() — nested helper inside get_some_cycle_from_scc; covered transitively by testing the outer method.

Already Covered

  • (none — no test files exist in this repository)

Recommendation
This repository has no test infrastructure (no tests/ directory, no conftest.py, no pytest configuration). Consider:

  1. Adding a tests/ directory with a test_conflict_resolving.py file.
  2. Adding a pytest.ini or pyproject.toml [tool.pytest] section.
  3. Writing the 5 test cases suggested in the existing inline comment for get_some_cycle_from_scc.

Review by automated test-coverage agent. Advisory only — no approval or changes requested.

@aircode610
Copy link
Copy Markdown
Owner

🤖 Test Coverage Review

Summary: 1 function added or modified without corresponding tests.

Missing Tests

File Line Symbol Status
conflict_resolving.py 268 get_some_cycle_from_scc() ⚠️ No test — inline comment already exists from prior review

Skipped Files

  • streamlit_app.py — Streamlit UI layer; changes are inside prose_tab() which is a UI callback and not independently unit-testable without heavy mocking. No new standalone functions or classes were added.

Notes

  • No test files exist anywhere in the repository (tests/test_conflict_resolving.py, test_conflict_resolving.py — both missing).
  • An inline review comment with 5 concrete test-case suggestions for get_some_cycle_from_scc was already posted in a previous review — no duplicate comment posted.
  • Recommended action: create tests/test_conflict_resolving.py and add the suggested test cases covering happy path (simple cycle), single-node SCC with/without self-loop, larger SCC with multiple cycles, and the empty-SCC edge case.

Review by automated test-coverage agent. Advisory only — no approval or change request.

@aircode610
Copy link
Copy Markdown
Owner

🤖 Test Coverage Review

Summary: 1 function added or modified without corresponding tests.

Missing Tests

File Line Symbol Status
conflict_resolving.py 268 get_some_cycle_from_scc() ⚠️ Inline comment already posted in a prior review — see existing comment

Skipped Files

  • streamlit_app.py — All changes are inside the prose_tab() Streamlit UI callback, which is tightly coupled to Streamlit session state and not independently unit-testable without significant mocking. Skipped per Streamlit exclusion rule.

Notes

  • No test files exist anywhere in the repository (tests/test_conflict_resolving.py, test_conflict_resolving.py — both missing).
  • The existing inline review comment on get_some_cycle_from_scc (line 268) already contains 5 concrete test case suggestions. No duplicate comment was posted.
  • Recommendation: Create tests/test_conflict_resolving.py and implement the suggested test cases from the existing inline comment. This is the highest-value testing target in this PR.

Review by automated test-coverage agent. Advisory only — no approval or change request made.

@aircode610
Copy link
Copy Markdown
Owner

🤖 Test Coverage Review

Summary: 1 function added or modified without corresponding tests.

Missing Tests:

Location Symbol Note
conflict_resolving.py:268 get_some_cycle_from_scc() Inline comment already exists from prior review — see discussion

Already Covered:

  • (none — no test files found in the repository)

Skipped Files:

  • streamlit_app.py — Skipped per entry-point / UI glue code rule (Streamlit app with UI callbacks tightly coupled to st.session_state; not independently unit-testable without extensive mocking).

⚠️ No test infrastructure detected.
No test files or test directories were found in this repository. Consider setting up test infrastructure:

  • Create a tests/ directory
  • Add pytest to your development dependencies (e.g., in requirements-dev.txt or pyproject.toml)
  • Add a conftest.py for shared fixtures
  • Consider adding a CI step to run tests automatically on PRs

get_some_cycle_from_scc is a pure-logic DFS-based cycle detection method and is an excellent candidate for unit testing. See the existing inline comment for 5 concrete test case suggestions (happy path, self-loop, no-loop, large SCC, empty input).


Review by automated test-coverage agent. Advisory only — no approval or change request submitted.

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