Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions culprit_finder/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ culprit-finder --repo <OWNER/REPO> --start <GOOD_SHA> --end <BAD_SHA> --workflow
- `--start`: The full or short SHA of the last known **good** commit.
- `--end`: The full or short SHA of the first known **bad** commit.
- `--workflow`: The filename of the GitHub Actions workflow to run (e.g., `ci.yml`, `tests.yaml`).
- `--dry-run`: (Optional) Simulates the bisection process by printing the API calls
that would be made without actually executing them.
- `--clear-cache`: (Optional) Deletes the local state file before execution to start a fresh bisection.

### State Persistence and Resuming
Expand Down
32 changes: 20 additions & 12 deletions culprit_finder/src/culprit_finder/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@ def main() -> None:
action="store_true",
help="Deletes the local state file before execution",
)
parser.add_argument(
"--dry-run",
action="store_true",
help="Simulates the bisection process by printing the API calls that would be made without actually executing them",
)

args = parser.parse_args()

Expand All @@ -74,13 +79,22 @@ def main() -> None:
if args.clear_cache and state_persister.exists():
state_persister.delete()

if state_persister.exists():
state: culprit_finder_state.CulpritFinderState = {
"repo": args.repo,
"workflow": args.workflow,
"original_start": args.start,
"original_end": args.end,
"current_good": "",
"current_bad": "",
"cache": {},
}
if state_persister.exists() and not args.dry_run:
print("\nA previous bisection state was found.")
resume = input("Do you want to resume from the saved state? (y/n): ").lower()
if resume not in ["y", "yes"]:
print("Starting a new bisection. Deleting the old state...")
state_persister.delete()
state: culprit_finder_state.CulpritFinderState = {
state = {
"repo": args.repo,
"workflow": args.workflow,
"original_start": args.start,
Expand All @@ -92,16 +106,6 @@ def main() -> None:
else:
state = state_persister.load()
print("Resuming from the saved state.")
else:
state: culprit_finder_state.CulpritFinderState = {
"repo": args.repo,
"workflow": args.workflow,
"original_start": args.start,
"original_end": args.end,
"current_good": "",
"current_bad": "",
"cache": {},
}

logging.info("Initializing culprit finder for %s", args.repo)
logging.info("Start commit: %s", args.start)
Expand All @@ -124,10 +128,14 @@ def main() -> None:
github_client=gh_client,
state=state,
state_persister=state_persister,
dry_run=args.dry_run,
)

try:
culprit_commit = finder.run_bisection()
if args.dry_run:
return

if culprit_commit:
commit_message = culprit_commit["message"].splitlines()[0]
print(
Expand Down
41 changes: 30 additions & 11 deletions culprit_finder/src/culprit_finder/culprit_finder.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ def __init__(
github_client: github.GithubClient,
state: culprit_finder_state.CulpritFinderState,
state_persister: culprit_finder_state.StatePersister,
dry_run: bool = False,
):
"""
Initializes the CulpritFinder instance.
Expand All @@ -40,6 +41,7 @@ def __init__(
github_client: The GithubClient instance used to interact with GitHub.
state: The CulpritFinderState object containing the current bisection state.
state_persister: The StatePersister object used to save the bisection state.
dry_run: Whether to run in dry-run mode (no API calls).
"""
self._repo = repo
self._start_sha = start_sha
Expand All @@ -50,6 +52,7 @@ def __init__(
self._gh_client = github_client
self._state = state
self._state_persister = state_persister
self._dry_run = dry_run

def _wait_for_workflow_completion(
self,
Expand Down Expand Up @@ -135,6 +138,15 @@ def _test_commit(
branch_name,
)

if self._dry_run:
logging.info(
"DRY RUN: Would trigger workflow %s on %s with inputs %s",
workflow_to_trigger,
branch_name,
inputs,
)
return True

# Get the ID of the previous run (if any) to distinguish it from the new one we are about to trigger
previous_run = self._gh_client.get_latest_run(workflow_to_trigger, branch_name)
previous_run_id = previous_run["databaseId"] if previous_run else None
Expand Down Expand Up @@ -202,29 +214,36 @@ def run_bisection(self) -> github.Commit | None:

# Ensure the branch does not exist from a previous run
if not self._gh_client.check_branch_exists(branch_name):
self._gh_client.create_branch(branch_name, commit_sha)
logging.info("Created branch %s", branch_name)
self._gh_client.wait_for_branch_creation(branch_name, timeout=180)
if self._dry_run:
logging.info("DRY RUN: Would create branch %s", branch_name)
else:
self._gh_client.create_branch(branch_name, commit_sha)
logging.info("Created branch %s", branch_name)
self._gh_client.wait_for_branch_creation(branch_name, timeout=180)

try:
is_good = self._test_commit(commit_sha, branch_name)
finally:
if self._gh_client.check_branch_exists(branch_name):
if self._dry_run:
logging.info("DRY RUN: Would delete branch %s", branch_name)
elif self._gh_client.check_branch_exists(branch_name):
logging.info("Deleting branch %s", branch_name)
self._gh_client.delete_branch(branch_name)

if is_good:
good_idx = mid_idx
self._state["current_good"] = commit_sha
self._state["cache"][commit_sha] = "PASS"
logging.info("Commit %s is good", commit_sha)
if not self._dry_run:
self._state["current_good"] = commit_sha
self._state["cache"][commit_sha] = "PASS"
else:
bad_idx = mid_idx
self._state["current_bad"] = commit_sha
self._state["cache"][commit_sha] = "FAIL"
logging.info("Commit %s is bad", commit_sha)
if not self._dry_run:
self._state["current_bad"] = commit_sha
self._state["cache"][commit_sha] = "FAIL"

self._state_persister.save(self._state)
if not self._dry_run:
logging.info("Commit %s is %s", commit_sha, "good" if is_good else "bad")
self._state_persister.save(self._state)

if bad_idx == len(commits):
return None
Expand Down
2 changes: 2 additions & 0 deletions culprit_finder/tests/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ def test_cli_success(
workflow_file="test.yml",
has_culprit_finder_workflow=has_culprit_workflow,
github_client=mock_gh_client_instance,
dry_run=False,
state=expected_state,
state_persister=patches["state_persister_inst"],
)
Expand Down Expand Up @@ -310,6 +311,7 @@ def test_cli_state_management(
state=existing_state,
github_client=mock_gh_client_instance,
state_persister=patches["state_persister_inst"],
dry_run=False,
)
else:
# If not exists or discarded, new state created
Expand Down
80 changes: 74 additions & 6 deletions culprit_finder/tests/test_culprit_finder.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
"""Tests for the CulpritFinder class."""

from culprit_finder import culprit_finder, github
from culprit_finder import culprit_finder_state
from culprit_finder import culprit_finder, culprit_finder_state, github
import re
import pytest
from datetime import datetime, timezone
Expand All @@ -24,9 +23,8 @@ def mock_state_persister(mocker):


@pytest.fixture
def finder(request, mock_gh_client, mock_state_persister):
"""Returns a CulpritFinder instance for testing."""
state: culprit_finder_state.CulpritFinderState = {
def mock_state() -> culprit_finder_state.CulpritFinderState:
return {
"repo": "test_repo",
"workflow": "test_workflow",
"original_start": "original_start_sha",
Expand All @@ -35,6 +33,11 @@ def finder(request, mock_gh_client, mock_state_persister):
"current_bad": "",
"cache": {},
}


@pytest.fixture
def finder(request, mock_gh_client, mock_state_persister, mock_state):
"""Returns a CulpritFinder instance for testing."""
has_culprit_finder_workflow = getattr(request, "param", True)
return culprit_finder.CulpritFinder(
repo=REPO,
Expand All @@ -43,7 +46,7 @@ def finder(request, mock_gh_client, mock_state_persister):
workflow_file=WORKFLOW_FILE,
has_culprit_finder_workflow=has_culprit_finder_workflow,
github_client=mock_gh_client,
state=state,
state=mock_state,
state_persister=mock_state_persister,
)

Expand Down Expand Up @@ -205,6 +208,7 @@ def test_run_bisection(
):
"""Tests various bisection scenarios including finding a culprit, no culprit, etc."""
mock_gh_client.compare_commits.return_value = commits

# Mock check_branch_exists to alternate False/True to simulate creation/deletion needs
# We need enough values for the max possible iterations (2 * len(commits))
mock_gh_client.check_branch_exists.side_effect = [False, True] * (len(commits) + 1)
Expand Down Expand Up @@ -346,3 +350,67 @@ def test_run_bisection_skips_testing_cached_commit(mocker, finder, mock_gh_clien
finder._state_persister.save.assert_called_once()
saved_state = finder._state_persister.save.call_args[0][0]
assert saved_state["cache"]["c2"] == "FAIL"


def test_run_bisection_dry_run(mock_gh_client, mock_state, mock_state_persister):
"""Tests that run_bisection in dry_run mode does not make state-changing API calls."""
commits = [
{"sha": "c0", "message": "m0"},
{"sha": "c1", "message": "m1"},
{"sha": "c2", "message": "m2"},
]
mock_gh_client.compare_commits.return_value = commits

# Simulate branch not existing initially
mock_gh_client.check_branch_exists.return_value = False

finder = culprit_finder.CulpritFinder(
repo=REPO,
start_sha="start_sha",
end_sha="end_sha",
workflow_file=WORKFLOW_FILE,
has_culprit_finder_workflow=True,
github_client=mock_gh_client,
dry_run=True,
state=mock_state,
state_persister=mock_state_persister,
)

# In dry run, _test_commit returns True, so no culprit should be found (all good)
result = finder.run_bisection()

assert result is None

# Verify API calls that change state were NOT made
mock_gh_client.create_branch.assert_not_called()
mock_gh_client.delete_branch.assert_not_called()
mock_gh_client.trigger_workflow.assert_not_called()

# Verify read-only calls were made
mock_gh_client.compare_commits.assert_called_once()
# check_branch_exists is called multiple times (before create, after test)
assert mock_gh_client.check_branch_exists.call_count > 0


def test_test_commit_dry_run(mock_gh_client, mock_state, mock_state_persister):
"""Tests that _test_commit returns True and makes no API calls in dry run mode."""
finder = culprit_finder.CulpritFinder(
repo=REPO,
start_sha="start_sha",
end_sha="end_sha",
workflow_file=WORKFLOW_FILE,
has_culprit_finder_workflow=True,
github_client=mock_gh_client,
dry_run=True,
state=mock_state,
state_persister=mock_state_persister,
)

branch = "test-branch"
commit_sha = "sha1"

result = finder._test_commit(commit_sha, branch)

assert result is True
mock_gh_client.trigger_workflow.assert_not_called()
mock_gh_client.get_latest_run.assert_not_called()