-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat(improve): load repo best_practices.md in OSS (#2377) #2382
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -174,6 +174,21 @@ def get_repo_settings(self): | |
| get_logger().error(f"Failed to get repo settings, error: {e}") | ||
| return "" | ||
|
|
||
| def get_pr_agent_repo_custom_file(self, file_path: str) -> bytes: | ||
| try: | ||
| contents = self.azure_devops_client.get_item_content( | ||
| repository_id=self.repo_slug, | ||
| project=self.workspace_slug, | ||
| download=False, | ||
| include_content_metadata=False, | ||
| include_content=True, | ||
| path=file_path, | ||
| ) | ||
| chunks = [c if isinstance(c, (bytes, bytearray)) else str(c).encode("utf-8") for c in contents] | ||
| return b"".join(chunks) | ||
|
Comment on lines
+187
to
+188
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2. Azure chunks comprehension too long The added list comprehension for chunks is on a single line that exceeds the repository’s 120-character limit, risking Ruff (E501) failures and reducing readability. Agent Prompt
|
||
| except Exception: | ||
| return b"" | ||
|
|
||
| def get_files(self): | ||
| files = [] | ||
| for i in self.azure_devops_client.get_pull_request_commits( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| import unittest | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| from pr_agent.tools.pr_code_suggestions import _load_repo_best_practices_md | ||
|
|
||
|
|
||
| def _provider(returns): | ||
| p = MagicMock(spec=["get_pr_agent_repo_custom_file"]) | ||
| p.get_pr_agent_repo_custom_file.return_value = returns | ||
| return p | ||
|
|
||
|
|
||
| class _FakeContext(dict): | ||
| """Mimics starlette_context.context.get/__setitem__ in a plain dict.""" | ||
|
|
||
|
|
||
| class _FakeContextProxy: | ||
| """Module-level proxy that works as both subscriptable and attribute target.""" | ||
|
|
||
| def __init__(self): | ||
| self._store = {} | ||
|
|
||
| def get(self, key, default=None): | ||
| return self._store.get(key, default) | ||
|
|
||
| def __getitem__(self, key): | ||
| return self._store[key] | ||
|
|
||
| def __setitem__(self, key, value): | ||
| self._store[key] = value | ||
|
|
||
| def reset(self): | ||
| self._store.clear() | ||
|
|
||
|
|
||
| class TestLoadRepoBestPracticesMd(unittest.TestCase): | ||
| def setUp(self): | ||
| self.fake_ctx = _FakeContextProxy() | ||
| self.ctx_patch = patch( | ||
| "pr_agent.tools.pr_code_suggestions.context", self.fake_ctx | ||
| ) | ||
| self.ctx_patch.start() | ||
|
|
||
| def tearDown(self): | ||
| self.ctx_patch.stop() | ||
|
|
||
| @patch("pr_agent.tools.pr_code_suggestions.get_settings") | ||
| def test_enabled_by_default_with_content(self, mock_get_settings): | ||
| s = MagicMock() | ||
| s.get.side_effect = lambda key, default=None: { | ||
| "best_practices.enable_repo_best_practices_md": True, | ||
| "best_practices.repo_best_practices_md_path": "best_practices.md", | ||
| "best_practices.max_lines_allowed": 800, | ||
| }.get(key, default) | ||
| mock_get_settings.return_value = s | ||
| prov = _provider(b"# Best practices\n- rule 1\n- rule 2\n") | ||
| out = _load_repo_best_practices_md(prov) | ||
| self.assertIn("rule 1", out) | ||
| self.assertIn("rule 2", out) | ||
| prov.get_pr_agent_repo_custom_file.assert_called_once_with("best_practices.md") | ||
|
|
||
| @patch("pr_agent.tools.pr_code_suggestions.get_settings") | ||
| def test_opt_out_skips_fetch(self, mock_get_settings): | ||
| s = MagicMock() | ||
| s.get.side_effect = lambda key, default=None: { | ||
| "best_practices.enable_repo_best_practices_md": False, | ||
| }.get(key, default) | ||
| mock_get_settings.return_value = s | ||
| prov = _provider(b"should not be read") | ||
| out = _load_repo_best_practices_md(prov) | ||
| self.assertEqual(out, "") | ||
| prov.get_pr_agent_repo_custom_file.assert_not_called() | ||
|
|
||
| @patch("pr_agent.tools.pr_code_suggestions.get_settings") | ||
| def test_file_absent_returns_empty(self, mock_get_settings): | ||
| s = MagicMock() | ||
| s.get.side_effect = lambda key, default=None: { | ||
| "best_practices.enable_repo_best_practices_md": True, | ||
| "best_practices.repo_best_practices_md_path": "best_practices.md", | ||
| "best_practices.max_lines_allowed": 800, | ||
| }.get(key, default) | ||
| mock_get_settings.return_value = s | ||
| prov = _provider(b"") | ||
| out = _load_repo_best_practices_md(prov) | ||
| self.assertEqual(out, "") | ||
|
|
||
| @patch("pr_agent.tools.pr_code_suggestions.get_logger") | ||
| @patch("pr_agent.tools.pr_code_suggestions.get_settings") | ||
| def test_truncation_emits_warning(self, mock_get_settings, mock_get_logger): | ||
| s = MagicMock() | ||
| s.get.side_effect = lambda key, default=None: { | ||
| "best_practices.enable_repo_best_practices_md": True, | ||
| "best_practices.repo_best_practices_md_path": "best_practices.md", | ||
| "best_practices.max_lines_allowed": 5, | ||
| }.get(key, default) | ||
| mock_get_settings.return_value = s | ||
| logger = MagicMock() | ||
| mock_get_logger.return_value = logger | ||
| body = "\n".join(f"line {i}" for i in range(20)) | ||
| prov = _provider(body.encode("utf-8")) | ||
| out = _load_repo_best_practices_md(prov) | ||
| self.assertEqual(len(out.splitlines()), 5) | ||
| # WARNING message about truncation must include the from/to counts. | ||
| warning_msgs = [c.args[0] for c in logger.warning.call_args_list] | ||
| self.assertTrue(any("Truncating" in m and "20" in m and "5" in m for m in warning_msgs), | ||
| f"warning not emitted: {warning_msgs}") | ||
| # INFO log emitted on fetch. | ||
| info_msgs = [c.args[0] for c in logger.info.call_args_list] | ||
| self.assertTrue(any("Loaded" in m for m in info_msgs)) | ||
|
|
||
| @patch("pr_agent.tools.pr_code_suggestions.get_settings") | ||
| def test_caches_across_calls(self, mock_get_settings): | ||
| s = MagicMock() | ||
| s.get.side_effect = lambda key, default=None: { | ||
| "best_practices.enable_repo_best_practices_md": True, | ||
| "best_practices.repo_best_practices_md_path": "best_practices.md", | ||
| "best_practices.max_lines_allowed": 800, | ||
| }.get(key, default) | ||
| mock_get_settings.return_value = s | ||
| prov = _provider(b"hello\n") | ||
| first = _load_repo_best_practices_md(prov) | ||
| second = _load_repo_best_practices_md(prov) | ||
| self.assertEqual(first, second) | ||
| prov.get_pr_agent_repo_custom_file.assert_called_once() | ||
|
|
||
| @patch("pr_agent.tools.pr_code_suggestions.get_settings") | ||
| def test_str_return_tolerated(self, mock_get_settings): | ||
| s = MagicMock() | ||
| s.get.side_effect = lambda key, default=None: { | ||
| "best_practices.enable_repo_best_practices_md": True, | ||
| "best_practices.repo_best_practices_md_path": "best_practices.md", | ||
| "best_practices.max_lines_allowed": 800, | ||
| }.get(key, default) | ||
| mock_get_settings.return_value = s | ||
| prov = _provider("text content\n") | ||
| out = _load_repo_best_practices_md(prov) | ||
| self.assertIn("text content", out) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() | ||
|
Comment on lines
+1
to
+158
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 3. testloadrepobestpracticesmd uses unittest The new test module is written using unittest.TestCase and unittest.main(), while the repository’s test suite is configured for pytest conventions. This can lead to inconsistent test patterns and maintenance overhead. Agent Prompt
|
||
Uh oh!
There was an error while loading. Please reload this page.