From f38c2faa9e8924098f27deeba407e34e1e9941a2 Mon Sep 17 00:00:00 2001 From: Wilson Freitas Date: Fri, 14 Aug 2026 07:04:57 -0300 Subject: [PATCH] speed up PR review workflow --- .github/workflows/pr-review.yml | 2 +- scripts/review_pr.py | 56 ++++++++++-- tests/test_pr_review_workflow.py | 2 +- tests/test_review_pr.py | 149 ++++++++++++++++++++++++++++++- 4 files changed, 198 insertions(+), 11 deletions(-) diff --git a/.github/workflows/pr-review.yml b/.github/workflows/pr-review.yml index f388a14..7204a90 100644 --- a/.github/workflows/pr-review.yml +++ b/.github/workflows/pr-review.yml @@ -34,4 +34,4 @@ jobs: GITHUB_TOKEN: ${{ github.token }} GITHUB_REPOSITORY: ${{ github.repository }} PR_NUMBER: ${{ github.event.pull_request.number }} - run: uv run python scripts/review_pr.py + run: uv run python scripts/review_pr.py --skip-pull-request-duplicates diff --git a/scripts/review_pr.py b/scripts/review_pr.py index 17ed486..a533790 100644 --- a/scripts/review_pr.py +++ b/scripts/review_pr.py @@ -71,6 +71,11 @@ def parse_args() -> argparse.Namespace: description="Review one awesome-quant pull request." ) parser.add_argument("--pr-number", type=int, default=None) + parser.add_argument( + "--skip-pull-request-duplicates", + action="store_true", + help="skip duplicate checks against open and recently closed PRs", + ) return parser.parse_args() @@ -252,7 +257,7 @@ class PinnedHTTPSConnection(http.client.HTTPSConnection): super().__init__( hostname, port=port, - timeout=10, + timeout=5, context=context, ) self.ip_address = ip_address @@ -382,9 +387,21 @@ def pull_request_has_duplicate( pull_request: Any, name: str, urls: list[str], + *, + readme_cache: dict[str, str] | None = None, ) -> bool: - base_readme = read_readme(repository, pull_request.base.sha) - head_readme = read_readme(repository, pull_request.head.sha) + cache = readme_cache if readme_cache is not None else {} + + def cached_readme(ref: str) -> str: + if ref not in cache: + cache[ref] = read_readme(repository, ref) + return cache[ref] + + head_readme = cached_readme(pull_request.head.sha) + if not readme_has_duplicate(head_readme, name, urls): + return False + + base_readme = cached_readme(pull_request.base.sha) added_lines, _removed_lines = readme_changed_lines(base_readme, head_readme) return any( entry_line_has_duplicate(line, name, urls) @@ -400,7 +417,9 @@ def repository_has_pull_request_duplicate( urls: list[str], *, now: datetime, + readme_cache: dict[str, str] | None = None, ) -> bool: + cache = readme_cache if readme_cache is not None else {} cutoff = now - timedelta(days=RECENT_CLOSED_PULL_DAYS) for state in ("open", "closed"): pulls = repository.get_pulls( @@ -417,7 +436,13 @@ def repository_has_pull_request_duplicate( closed_at = pull_request.closed_at if closed_at is None or closed_at < cutoff: continue - if pull_request_has_duplicate(repository, pull_request, name, urls): + if pull_request_has_duplicate( + repository, + pull_request, + name, + urls, + readme_cache=cache, + ): return True return False @@ -428,6 +453,7 @@ def review_pr( client: Github, *, now: datetime | None = None, + check_pull_request_duplicates: bool = True, ) -> tuple[list[Finding], str]: current_time = now or datetime.now(timezone.utc) @@ -558,19 +584,23 @@ def review_pr( ) ) - if not url_reachable(url): + if not primary_github and not url_reachable(url): findings.append(Finding("reachability", f"primary URL is not reachable: {url}")) if readme_has_duplicate(base_readme, name, [url, *github_urls]): findings.append( Finding("duplicates", "project name or URL already exists in README.md") ) - elif repository_has_pull_request_duplicate( + elif check_pull_request_duplicates and repository_has_pull_request_duplicate( repository, pr_number, name, [url, *github_urls], now=current_time, + readme_cache={ + pull_request.base.sha: base_readme, + pull_request.head.sha: head_readme, + }, ): findings.append( Finding( @@ -606,15 +636,25 @@ def main() -> int: if not repository_name: return fail("GITHUB_REPOSITORY is required") client = Github(auth=Auth.Token(token)) - findings, title = review_pr(repository_name, pr_number, client) + findings, title = review_pr( + repository_name, + pr_number, + client, + check_pull_request_duplicates=not args.skip_pull_request_duplicates, + ) except Exception as exc: return fail(str(exc)) print(f"PR #{pr_number}: {title}") print("Entries reviewed: 1") + if args.skip_pull_request_duplicates: + print("Cross-PR duplicate check: skipped") if not findings: for check in CHECK_ORDER: - print(f"- {check}: pass") + if check == "duplicates" and args.skip_pull_request_duplicates: + print("- duplicates: pass (existing README only)") + else: + print(f"- {check}: pass") print("Verdict: APPROVE") print("Recommended action: merge") return 0 diff --git a/tests/test_pr_review_workflow.py b/tests/test_pr_review_workflow.py index d691fef..d996967 100644 --- a/tests/test_pr_review_workflow.py +++ b/tests/test_pr_review_workflow.py @@ -40,7 +40,7 @@ jobs: GITHUB_TOKEN: ${{ github.token }} GITHUB_REPOSITORY: ${{ github.repository }} PR_NUMBER: ${{ github.event.pull_request.number }} - run: uv run python scripts/review_pr.py + run: uv run python scripts/review_pr.py --skip-pull-request-duplicates """ diff --git a/tests/test_review_pr.py b/tests/test_review_pr.py index 43357b0..25396af 100644 --- a/tests/test_review_pr.py +++ b/tests/test_review_pr.py @@ -10,6 +10,7 @@ from github import GithubException from scripts.review_pr import ( Finding, + PinnedHTTPSConnection, main, readme_has_duplicate, review_pr, @@ -373,6 +374,36 @@ class PullRequestDuplicateTests(unittest.TestCase): self.assertIn("duplicates", {finding.check for finding in findings}) + def test_unrelated_candidates_require_only_one_readme_request_each(self): + unrelated_patch = """@@ -1,1 +1,2 @@ + ## Trading & Backtesting ++- [Other](https://github.com/example/other) - `Python` - Other project. +""" + candidates = [ + FakePull( + number, + files=[SimpleNamespace(filename="README.md", patch=unrelated_patch)], + ) + for number in range(100, 200) + ] + + findings, _title, client = self.review(other_pulls=candidates) + + self.assertEqual(findings, []) + self.assertEqual(len(client.repository.content_refs), 102) + candidate_base_refs = {f"base-{number}" for number in range(100, 200)} + self.assertTrue( + candidate_base_refs.isdisjoint(client.repository.content_refs) + ) + + def test_reuses_the_current_base_readme_when_confirming_a_duplicate(self): + duplicate = FakePull(9, base_sha="base-sha") + + findings, _title, client = self.review(other_pulls=[duplicate]) + + self.assertIn("duplicates", {finding.check for finding in findings}) + self.assertEqual(client.repository.content_refs.count("base-sha"), 1) + def test_ignores_old_closed_pull_request(self): old_duplicate = FakePull( 7, @@ -449,6 +480,45 @@ class PullRequestDuplicateTests(unittest.TestCase): self.assertEqual(findings, []) + def test_can_skip_cross_pull_request_duplicate_scanning(self): + candidates = [ + FakePull(number, content_error=True) + for number in range(100, 200) + ] + repository = FakeBaseRepository(FakePull(10), other_pulls=candidates) + client = FakeClient(repository) + + with patch("scripts.review_pr.url_reachable", return_value=True): + findings, _title = review_pr( + "owner/list", + 10, + client, + now=NOW, + check_pull_request_duplicates=False, + ) + + self.assertEqual(findings, []) + self.assertEqual(repository.content_refs, ["base-sha", "head-sha"]) + + def test_skip_still_rejects_a_duplicate_in_the_base_readme(self): + base_readme = ( + "## Trading & Backtesting\n" + "- [Fresh](https://github.com/example/old) - `Python` - Existing project.\n" + ) + repository = FakeBaseRepository(FakePull(10), base_readme=base_readme) + client = FakeClient(repository) + + with patch("scripts.review_pr.url_reachable", return_value=True): + findings, _title = review_pr( + "owner/list", + 10, + client, + now=NOW, + check_pull_request_duplicates=False, + ) + + self.assertIn("duplicates", {finding.check for finding in findings}) + def test_pull_request_search_errors_fail_closed(self): repository = FakeBaseRepository(FakePull(10)) repository.pull_query_error = RuntimeError("pull search failed") @@ -530,6 +600,41 @@ class ValidationPipelineTests(unittest.TestCase): self.assertIn("head-sha", repository.content_refs) self.assertNotIn("main", repository.content_refs) + def test_does_not_probe_a_github_primary_url_after_api_validation(self): + repository = FakeBaseRepository(FakePull(10)) + client = FakeClient(repository) + + with patch("scripts.review_pr.url_reachable") as url_checker: + findings, _title = review_pr("owner/list", 10, client, now=NOW) + + self.assertEqual(findings, []) + url_checker.assert_not_called() + + def test_still_probes_an_external_primary_url(self): + patch_text = """@@ -1,1 +1,2 @@ + ## Trading & Backtesting ++- [Fresh](https://example.com/fresh) - `Python` - Fresh project. [GitHub](https://github.com/example/fresh) +""" + base_readme = "# awesome-quant\n\n## Trading & Backtesting\n" + head_readme = base_readme + patch_text.splitlines()[-1][1:] + "\n" + repository = FakeBaseRepository( + FakePull(10, files=[SimpleNamespace(filename="README.md", patch=patch_text)]), + base_readme=base_readme, + head_readme=head_readme, + ) + client = FakeClient(repository) + + with patch("scripts.review_pr.url_reachable", return_value=True) as url_checker: + findings, _title = review_pr("owner/list", 10, client, now=NOW) + + self.assertEqual(findings, []) + url_checker.assert_called_once_with("https://example.com/fresh") + + def test_url_connections_use_a_short_timeout(self): + connection = PinnedHTTPSConnection("example.com", "93.184.216.34", 443) + + self.assertEqual(connection.timeout, 5) + def test_accepts_entry_when_heading_is_outside_patch_context(self): patch_text = """@@ -20,0 +21,1 @@ +- [Fresh](https://github.com/example/fresh) - `Python` - Fresh project. @@ -748,7 +853,15 @@ class ValidationPipelineTests(unittest.TestCase): self.review(configure_project=fail_readme_lookup) def test_rejects_unreachable_primary_url(self): - self.assertIn("reachability", self.review(reachable=False)) + patch_text = """@@ -1,1 +1,2 @@ + ## Trading & Backtesting ++- [Fresh](https://example.com/fresh) - `Python` - Fresh project. [GitHub](https://github.com/example/fresh) +""" + + self.assertIn( + "reachability", + self.review(patch_text=patch_text, reachable=False), + ) def test_rejects_duplicate_in_base_readme(self): base_readme = ( @@ -789,6 +902,40 @@ class MainTests(unittest.TestCase): self.assertIn("description: pass", stdout) self.assertIn("duplicates: pass", stdout) + def test_skip_flag_is_reported_and_forwarded(self): + stdout = io.StringIO() + stderr = io.StringIO() + environment = { + "GITHUB_TOKEN": "token", + "GITHUB_REPOSITORY": "owner/list", + "PR_NUMBER": "10", + } + with ( + patch.dict("os.environ", environment, clear=True), + patch( + "sys.argv", + ["review_pr.py", "--skip-pull-request-duplicates"], + ), + patch("scripts.review_pr.Github"), + patch( + "scripts.review_pr.review_pr", + return_value=([], "Add Fresh"), + ) as reviewer, + redirect_stdout(stdout), + redirect_stderr(stderr), + ): + result = main() + + self.assertEqual(result, 0) + self.assertIn("Cross-PR duplicate check: skipped", stdout.getvalue()) + self.assertIn( + "duplicates: pass (existing README only)", + stdout.getvalue(), + ) + self.assertFalse( + reviewer.call_args.kwargs["check_pull_request_duplicates"] + ) + def test_authentication_does_not_emit_a_deprecation_warning(self): stdout = io.StringIO() stderr = io.StringIO()