diff --git a/culprit_finder/README.md b/culprit_finder/README.md index 8edea27..b350957 100644 --- a/culprit_finder/README.md +++ b/culprit_finder/README.md @@ -58,6 +58,7 @@ culprit-finder --repo --start --end --workflow - `--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`). - `--clear-cache`: (Optional) Deletes the local state file before execution to start a fresh bisection. +- `--no-cache`: (Optional) Disabled cached results. This will run the workflow on all commits. ### State Persistence and Resuming diff --git a/culprit_finder/src/culprit_finder/cli.py b/culprit_finder/src/culprit_finder/cli.py index 37d7668..8cb09bb 100644 --- a/culprit_finder/src/culprit_finder/cli.py +++ b/culprit_finder/src/culprit_finder/cli.py @@ -55,6 +55,11 @@ def main() -> None: action="store_true", help="Deletes the local state file before execution", ) + parser.add_argument( + "--no-cache", + action="store_true", + help="Disabled cached results. This will run the workflow on all commits.", + ) args = parser.parse_args() @@ -67,6 +72,7 @@ def main() -> None: logging.error("Not authenticated with GitHub CLI or GH_TOKEN env var is not set.") sys.exit(1) + use_cache = not args.no_cache state_persister = culprit_finder_state.StatePersister( repo=args.repo, workflow=args.workflow ) @@ -107,6 +113,7 @@ def main() -> None: logging.info("Start commit: %s", args.start) logging.info("End commit: %s", args.end) logging.info("Workflow: %s", args.workflow) + logging.info("Use cache: %s", use_cache) has_culprit_finder_workflow = any( wf["path"] == ".github/workflows/culprit_finder.yml" @@ -124,6 +131,7 @@ def main() -> None: github_client=gh_client, state=state, state_persister=state_persister, + use_cache=use_cache, ) try: diff --git a/culprit_finder/src/culprit_finder/culprit_finder.py b/culprit_finder/src/culprit_finder/culprit_finder.py index ce29408..d1a9e45 100644 --- a/culprit_finder/src/culprit_finder/culprit_finder.py +++ b/culprit_finder/src/culprit_finder/culprit_finder.py @@ -27,6 +27,7 @@ def __init__( github_client: github.GithubClient, state: culprit_finder_state.CulpritFinderState, state_persister: culprit_finder_state.StatePersister, + use_cache: bool = True, ): """ Initializes the CulpritFinder instance. @@ -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. + use_cache: Whether to use the cached results from previous runs. Defaults to True. """ self._repo = repo self._start_sha = start_sha @@ -50,6 +52,7 @@ def __init__( self._gh_client = github_client self._state = state self._state_persister = state_persister + self._use_cache = use_cache def _wait_for_workflow_completion( self, @@ -75,7 +78,9 @@ def _wait_for_workflow_completion( """ start_time = time.time() while time.time() - start_time < timeout: - latest_run = self._gh_client.get_latest_run(workflow_file, branch_name) + latest_run = self._gh_client.get_latest_run( + workflow_file=workflow_file, branch=branch_name, event="workflow_dispatch" + ) if not latest_run: logging.info( @@ -136,7 +141,9 @@ def _test_commit( ) # 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 = self._gh_client.get_latest_run( + workflow_file=workflow_to_trigger, branch=branch_name, event="workflow_dispatch" + ) previous_run_id = previous_run["databaseId"] if previous_run else None self._gh_client.trigger_workflow( @@ -157,6 +164,68 @@ def _test_commit( return run["conclusion"] == "success" + def _check_existing_run(self, commit_sha: str) -> bool | None: + """ + Checks for an existing workflow run for the commit. + + Args: + commit_sha: The SHA of the commit to check for existing runs. + + Returns: + True if a successful run is found, False if a failed run is found, + or None if no completed run exists. + """ + previous_run = self._gh_client.get_latest_run( + workflow_file=self._workflow_file, commit=commit_sha, status="completed" + ) + if previous_run: + logging.info( + "Found result from previous run for commit %s, skipping test", commit_sha + ) + return previous_run["conclusion"] == "success" + return None + + def _execute_test_with_branch(self, commit_sha: str) -> bool: + """ + Creates a branch, runs the test, and cleans up. + + Args: + commit_sha: The SHA of the commit to be tested. + + Returns: + True if the test passed, False otherwise. + """ + branch_name = f"culprit-finder/test-{commit_sha}_{uuid.uuid4()}" + + # 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) + + try: + return self._test_commit(commit_sha, branch_name) + finally: + if self._gh_client.check_branch_exists(branch_name): + logging.info("Deleting branch %s", branch_name) + self._gh_client.delete_branch(branch_name) + + def _update_state(self, commit_sha: str, is_good: bool) -> None: + """ + Updates the state and persists it. + + Args: + commit_sha: The SHA of the commit that was tested. + is_good: Whether the commit was identified as good (True) or bad (False). + """ + if is_good: + self._state["current_good"] = commit_sha + self._state["cache"][commit_sha] = "PASS" + else: + self._state["current_bad"] = commit_sha + self._state["cache"][commit_sha] = "FAIL" + self._state_persister.save(self._state) + def run_bisection(self) -> github.Commit | None: """ Runs bisection logic (binary search) to find the culprit commit for a GitHub workflow. @@ -184,47 +253,29 @@ def run_bisection(self) -> github.Commit | None: while bad_idx - good_idx > 1: mid_idx = (good_idx + bad_idx) // 2 commit_sha = commits[mid_idx]["sha"] + is_good = None + is_cached = False if commit_sha in self._state["cache"]: logging.info("Using cached result for commit %s", commit_sha) is_good = self._state["cache"][commit_sha] == "PASS" + is_cached = True - if is_good: - good_idx = mid_idx - logging.info("Commit %s is good", commit_sha) - else: - bad_idx = mid_idx - logging.info("Commit %s is bad", commit_sha) - - continue - - branch_name = f"culprit-finder/test-{commit_sha}_{uuid.uuid4()}" - - # 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 is_good is None and self._use_cache: + is_good = self._check_existing_run(commit_sha) - try: - is_good = self._test_commit(commit_sha, branch_name) - finally: - if 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 is None: + is_good = self._execute_test_with_branch(commit_sha) 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) 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) - self._state_persister.save(self._state) + if not is_cached: + self._update_state(commit_sha, is_good) if bad_idx == len(commits): return None diff --git a/culprit_finder/src/culprit_finder/github.py b/culprit_finder/src/culprit_finder/github.py index 02a6760..17afe5c 100644 --- a/culprit_finder/src/culprit_finder/github.py +++ b/culprit_finder/src/culprit_finder/github.py @@ -136,13 +136,24 @@ def trigger_workflow( self._run_command(cmd) - def get_latest_run(self, workflow_file: str, branch: str) -> Run | None: + def get_latest_run( + self, + workflow_file: str, + branch: Optional[str] = None, + commit: Optional[str] = None, + event: Optional[str] = None, + status: Optional[str] = None, + ) -> Run | None: """ Gets the latest workflow run for a specific branch and workflow. Args: - workflow_file: The filename or ID of the workflow to query. - branch: The git branch reference to filter runs by. + workflow_file: The filename or ID of the workflow to query (e.g., "build.yml" or "12345"). + branch: Optional. The git branch reference to filter runs by (e.g., "main", "feature-branch"). + commit: Optional. The commit SHA to filter runs by. + event: Optional. The workflow event type to filter runs by (e.g., "push", "pull_request", "workflow_dispatch"). + status: Optional. The run status to filter runs by (e.g., "completed", "in_progress", "queued", "failure"). + Returns: A dictionary representing the latest workflow run object (containing fields like @@ -154,10 +165,6 @@ def get_latest_run(self, workflow_file: str, branch: str) -> Run | None: "list", "--workflow", workflow_file, - "--branch", - branch, - "--event", - "workflow_dispatch", "--limit", "1", "--json", @@ -165,6 +172,14 @@ def get_latest_run(self, workflow_file: str, branch: str) -> Run | None: "--repo", self.repo, ] + if branch: + cmd.extend(["--branch", branch]) + if commit: + cmd.extend(["--commit", commit]) + if event: + cmd.extend(["--event", event]) + if status: + cmd.extend(["--status", status]) output = self._run_command(cmd) runs = json.loads(output) diff --git a/culprit_finder/tests/test_cli.py b/culprit_finder/tests/test_cli.py index 23e5c09..2b456be 100644 --- a/culprit_finder/tests/test_cli.py +++ b/culprit_finder/tests/test_cli.py @@ -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, + use_cache=True, state=expected_state, state_persister=patches["state_persister_inst"], ) @@ -310,6 +311,7 @@ def test_cli_state_management( state=existing_state, github_client=mock_gh_client_instance, state_persister=patches["state_persister_inst"], + use_cache=True, ) else: # If not exists or discarded, new state created diff --git a/culprit_finder/tests/test_culprit_finder.py b/culprit_finder/tests/test_culprit_finder.py index f2a7fd7..8cda2e6 100644 --- a/culprit_finder/tests/test_culprit_finder.py +++ b/culprit_finder/tests/test_culprit_finder.py @@ -24,9 +24,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", @@ -35,6 +34,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, @@ -43,7 +47,8 @@ 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, + use_cache=False, + state=mock_state, state_persister=mock_state_persister, ) @@ -88,7 +93,7 @@ def test_wait_for_workflow_completion_success(mocker, finder, mock_gh_client): assert mock_gh_client.get_latest_run.call_count == 3 for call_args in mock_gh_client.get_latest_run.call_args_list: - assert call_args[0][0] == workflow + assert call_args.kwargs["workflow_file"] == workflow @pytest.mark.parametrize("finder", [True, False], indirect=True) @@ -346,3 +351,184 @@ 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_uses_cache( + mocker, mock_gh_client, mock_state, mock_state_persister +): + """Tests that bisection uses cached results when available.""" + commits = [ + _create_commit("c0", "m0"), + _create_commit("c1", "m1"), + _create_commit("c2", "m2"), + ] + mock_gh_client.compare_commits.return_value = commits + + 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, + use_cache=True, + state=mock_state, + state_persister=mock_state_persister, + ) + + # Scenario: + # c1 is checked first (mid). Cache returns failure. -> bad_idx = 1 + # c0 is checked next. Cache returns success. -> good_idx = 0 + # Result: c1 is culprit. + + def get_latest_run_side_effect(**kwargs): + commit = kwargs.get("commit") + if kwargs.get("status") == "completed": + if commit == "c1": + return {"conclusion": "failure"} + if commit == "c0": + return {"conclusion": "success"} + return None + + mock_gh_client.get_latest_run.side_effect = get_latest_run_side_effect + + mock_test_commit = mocker.patch.object(finder, "_test_commit") + + culprit = finder.run_bisection() + + assert culprit == commits[1] + mock_test_commit.assert_not_called() + mock_gh_client.create_branch.assert_not_called() + + +def test_run_bisection_mixed_cache( + mocker, mock_gh_client, mock_state, mock_state_persister +): + """Tests bisection with a mix of cached and non-cached results.""" + commits = [_create_commit("c0", "m0"), _create_commit("c1", "m1")] + mock_gh_client.compare_commits.return_value = commits + + 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, + use_cache=True, + state=mock_state, + state_persister=mock_state_persister, + ) + + # Scenario: + # c0 is checked first (mid). Cache miss. Run test -> Success. + # c1 is checked next (bad_idx was 2, good_idx became 0, mid is 1). Cache hit -> Failure. + # Result: c1 is culprit. + + def get_latest_run_side_effect(**kwargs): + if kwargs.get("status") == "completed" and kwargs.get("commit") == "c1": + return {"conclusion": "failure"} + return None + + mock_gh_client.get_latest_run.side_effect = get_latest_run_side_effect + + # Mock _test_commit only for c0 (success) + mock_test_commit = mocker.patch.object(finder, "_test_commit") + mock_test_commit.side_effect = lambda sha, branch: True if sha == "c0" else False + + # Mock branch existence for c0 test + mock_gh_client.check_branch_exists.side_effect = [False, True] + + culprit = finder.run_bisection() + + assert culprit == commits[1] + + # verify _test_commit called for c0 but not c1 + assert mock_test_commit.call_count == 1 + assert mock_test_commit.call_args[0][0] == "c0" + + +def test_check_existing_run_found_success(finder, mock_gh_client): + """Tests _check_existing_run when a successful run exists.""" + mock_gh_client.get_latest_run.return_value = {"conclusion": "success"} + + result = finder._check_existing_run("sha1") + + assert result is True + mock_gh_client.get_latest_run.assert_called_once_with( + workflow_file=finder._workflow_file, commit="sha1", status="completed" + ) + + +def test_check_existing_run_found_failure(finder, mock_gh_client): + """Tests _check_existing_run when a failed run exists.""" + mock_gh_client.get_latest_run.return_value = {"conclusion": "failure"} + + result = finder._check_existing_run("sha1") + + assert result is False + + +def test_check_existing_run_not_found(finder, mock_gh_client): + """Tests _check_existing_run when no run exists.""" + mock_gh_client.get_latest_run.return_value = None + + result = finder._check_existing_run("sha1") + + assert result is None + + +def test_execute_test_with_branch_success(mocker, finder, mock_gh_client): + """Tests _execute_test_with_branch with successful execution.""" + commit_sha = "sha1" + + # Mock branch check: doesn't exist initially, exists for deletion + mock_gh_client.check_branch_exists.side_effect = [False, True] + + mock_test = mocker.patch.object(finder, "_test_commit", return_value=True) + + result = finder._execute_test_with_branch(commit_sha) + + assert result is True + mock_gh_client.create_branch.assert_called_once() + args, _ = mock_gh_client.create_branch.call_args + assert args[1] == commit_sha + assert "culprit-finder/test-sha1_" in args[0] + + mock_gh_client.wait_for_branch_creation.assert_called_once() + mock_test.assert_called_once() + mock_gh_client.delete_branch.assert_called_once() + + +def test_execute_test_with_branch_exception_cleanup(mocker, finder, mock_gh_client): + """Tests cleanup in _execute_test_with_branch when an exception occurs.""" + commit_sha = "sha1" + + mock_gh_client.check_branch_exists.side_effect = [False, True] + mocker.patch.object(finder, "_test_commit", side_effect=Exception("Test error")) + + with pytest.raises(Exception, match="Test error"): + finder._execute_test_with_branch(commit_sha) + + mock_gh_client.create_branch.assert_called_once() + mock_gh_client.delete_branch.assert_called_once() + + +def test_update_state_good(finder, mock_state_persister): + """Tests _update_state for a good commit.""" + commit_sha = "sha1" + finder._update_state(commit_sha, is_good=True) + + assert finder._state["current_good"] == commit_sha + assert finder._state["cache"][commit_sha] == "PASS" + mock_state_persister.save.assert_called_once_with(finder._state) + + +def test_update_state_bad(finder, mock_state_persister): + """Tests _update_state for a bad commit.""" + commit_sha = "sha1" + finder._update_state(commit_sha, is_good=False) + + assert finder._state["current_bad"] == commit_sha + assert finder._state["cache"][commit_sha] == "FAIL" + mock_state_persister.save.assert_called_once_with(finder._state)