test- inmproved test coverage of github internal nodes more than 80% … #2042
Conversation
Summary by CodeRabbit
WalkthroughAdds comprehensive unit tests for multiple GitHub GraphQL node types (Issue, Milestone, Organization, PullRequest, Release, Repository, User) exercising field resolvers, Strawberry definitions, and edge cases; no production code changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
backend/tests/apps/github/api/internal/nodes/user_test.py (2)
11-13: Nit: Clarify the test name/docstringDocstring says “inherits from BaseNode,” but the assertion checks for a Strawberry definition. Consider updating the docstring to reflect what’s actually verified.
- def test_user_node_inheritance(self): - """Test if UserNode inherits from BaseNode.""" + def test_user_node_is_strawberry_type(self): + """Test if UserNode is a Strawberry type (has __strawberry_definition__)."""
15-38: Avoid brittle schema assertions; use subset instead of equalitySchema may evolve. Asserting exact equality makes tests fragile. Prefer subset to ensure required fields exist without blocking additive changes.
- assert field_names == expected_field_names + assert expected_field_names.issubset(field_names)backend/tests/apps/github/api/internal/nodes/milestone_test.py (2)
14-29: Relax strict field equality to reduce flakinessUse subset to guard against future additive fields in the Strawberry type.
- assert field_names == expected_field_names + assert expected_field_names.issubset(field_names)
70-87: Add a rounding edge-case for progressConsider adding a case that exercises rounding (e.g., 1 closed, 2 open → 33.33) to assert the two-decimal behavior.
+ def test_progress_rounding(self): + """Test progress rounding to two decimals.""" + m = Mock() + m.closed_issues_count = 1 + m.open_issues_count = 2 + assert MilestoneNode.progress(m) == 33.33backend/tests/apps/github/api/internal/nodes/release_test.py (1)
15-28: Prefer subset over equality for field listsUse subset to future-proof tests against additive fields in the GraphQL type.
- assert field_names == expected_field_names + assert expected_field_names.issubset(field_names)backend/tests/apps/github/api/internal/nodes/issue_test.py (1)
14-25: Make schema checks resilientPrefer subset checks to avoid brittleness when fields are added.
- assert field_names == expected_field_names + assert expected_field_names.issubset(field_names)backend/tests/apps/github/api/internal/nodes/pull_request_test.py (1)
14-24: Reduce flakiness in schema assertionsUse subset instead of equality to handle additive schema changes gracefully.
- assert field_names == expected_field_names + assert expected_field_names.issubset(field_names)backend/tests/apps/github/api/internal/nodes/organization_test.py (1)
15-32: Optional: relax equality on schema fieldsIf the OrganizationNode schema gains fields, this test will start failing. Consider subset checks.
- assert field_names == expected_field_names + assert expected_field_names.issubset(field_names)backend/tests/apps/github/api/internal/nodes/repository_test.py (3)
57-61: Make list type assertions version-robustStrawberry/Python versions can differ in how list types are represented (list[str] vs typing.List[str]). Use typing.get_origin/get_args to avoid false negatives.
+from typing import get_origin, get_args, List ... - assert field.type == list[str] + assert get_origin(field.type) in {list, List} + assert get_args(field.type) in { (str,), (str,) } ... - assert field.type == list[str] + assert get_origin(field.type) in {list, List} + assert get_args(field.type) in { (str,), (str,) }Also applies to: 92-96
102-114: Verify slicing is invoked for issuesYou’re validating select_related/order_by; also assert that slicing occurred to ensure the limit behavior is exercised.
RepositoryNode.issues(mock_repository) mock_issues.select_related.assert_called_with("author") mock_issues.select_related.return_value.order_by.assert_called_with("-created_at") + mock_issues.select_related.return_value.order_by.return_value.__getitem__.assert_called()
170-179: Likewise, assert slicing for releasesAdd an assertion that slicing was performed after ordering by published_at.
RepositoryNode.releases(mock_repository) mock_releases.order_by.assert_called_with("-published_at") + mock_releases.order_by.return_value.__getitem__.assert_called()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/tests/apps/github/api/internal/nodes/issue_test.py(2 hunks)backend/tests/apps/github/api/internal/nodes/milestone_test.py(2 hunks)backend/tests/apps/github/api/internal/nodes/organization_test.py(3 hunks)backend/tests/apps/github/api/internal/nodes/pull_request_test.py(1 hunks)backend/tests/apps/github/api/internal/nodes/release_test.py(2 hunks)backend/tests/apps/github/api/internal/nodes/repository_test.py(1 hunks)backend/tests/apps/github/api/internal/nodes/user_test.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
backend/tests/apps/github/api/internal/nodes/pull_request_test.py (1)
backend/apps/github/api/internal/nodes/pull_request.py (1)
PullRequestNode(17-42)
backend/tests/apps/github/api/internal/nodes/user_test.py (3)
backend/apps/github/api/internal/nodes/user.py (6)
UserNode(27-53)created_at(31-33)issues_count(36-38)releases_count(41-43)updated_at(46-48)url(51-53)backend/apps/github/models/mixins/user.py (1)
idx_releases_count(172-174)backend/tests/apps/github/api/internal/nodes/release_test.py (1)
test_url_field(123-129)
backend/tests/apps/github/api/internal/nodes/release_test.py (1)
backend/apps/github/api/internal/nodes/release.py (1)
ReleaseNode(20-54)
backend/tests/apps/github/api/internal/nodes/organization_test.py (3)
backend/tests/apps/github/api/internal/queries/organization_test.py (1)
mock_organization(15-20)backend/apps/github/api/internal/nodes/organization.py (3)
OrganizationNode(39-76)stats(43-71)OrganizationStatsNode(13-20)backend/tests/apps/github/api/internal/nodes/repository_test.py (1)
test_url_method(197-203)
backend/tests/apps/github/api/internal/nodes/issue_test.py (3)
backend/tests/apps/github/api/internal/nodes/milestone_test.py (6)
test_author_field(31-38)test_organization_name_with_organization(40-50)test_organization_name_without_organization(52-60)test_organization_name_without_repository(62-68)test_repository_name_with_repository(88-96)test_repository_name_without_repository(98-104)backend/tests/apps/github/api/internal/nodes/pull_request_test.py (6)
test_author_field(26-33)test_organization_name_with_organization(35-45)test_organization_name_without_organization(47-55)test_organization_name_without_repository(57-63)test_repository_name_with_repository(65-73)test_repository_name_without_repository(75-81)backend/apps/github/api/internal/nodes/issue.py (1)
IssueNode(19-39)
backend/tests/apps/github/api/internal/nodes/milestone_test.py (1)
backend/apps/github/api/internal/nodes/milestone.py (2)
MilestoneNode(22-50)progress(40-45)
backend/tests/apps/github/api/internal/nodes/repository_test.py (5)
backend/apps/github/api/internal/nodes/issue.py (1)
IssueNode(19-39)backend/apps/github/api/internal/nodes/organization.py (1)
OrganizationNode(39-76)backend/apps/github/api/internal/nodes/release.py (1)
ReleaseNode(20-54)backend/apps/github/api/internal/nodes/repository.py (2)
RepositoryNode(40-100)owner_key(65-67)backend/apps/github/api/internal/nodes/repository_contributor.py (1)
RepositoryContributorNode(7-15)
🔇 Additional comments (12)
backend/tests/apps/github/api/internal/nodes/user_test.py (1)
40-78: Resolver tests look goodField resolver tests for created_at, issues_count, releases_count, updated_at, and url are straightforward and accurate.
backend/tests/apps/github/api/internal/nodes/milestone_test.py (1)
31-39: LGTM on resolver coverageAuthor, organization_name (present/absent), and repository_name (present/absent) test cases are precise and valuable.
Also applies to: 88-105
backend/tests/apps/github/api/internal/nodes/release_test.py (1)
36-44: LGTM on resolver testsAuthor, organization_name (present/absent), repository_name (present/absent), and url resolution tests are accurate.
Also applies to: 105-122, 123-129
backend/tests/apps/github/api/internal/nodes/issue_test.py (1)
27-83: LGTM on resolver coverageAuthor and organization_name/repository_name tests cover both present and absent nested attributes effectively.
backend/tests/apps/github/api/internal/nodes/pull_request_test.py (1)
83-97: LGTM on url resolver behaviorCorrectly asserts empty string when url is None and returns the given URL otherwise.
backend/tests/apps/github/api/internal/nodes/organization_test.py (3)
49-67: Patch targets and ordering look correctPatching Repository.objects and RepositoryContributor.objects at module paths used by OrganizationNode, with parameter order matching patch stacking, is correct.
82-116: Solid coverage for stats edge casesGood handling of both actual values and None aggregates (defaulting to 0). This meaningfully exercises the resolver logic.
126-138: Stats node fields assertion is fineExact field set on OrganizationStatsNode is appropriate here and improves schema safety.
backend/tests/apps/github/api/internal/nodes/repository_test.py (4)
17-45: Good use of subset for meta configurationUsing issubset here reduces brittleness while still enforcing key fields.
131-139: LGTM on resolver behavior testsDirect-return resolvers (latest_release, organization, owner_key, project, recent_milestones ordering, top_contributors, topics, url) are covered well and read clearly.
Also applies to: 140-147, 148-156, 157-169, 180-188, 189-203
52-56: Schema type checks look goodValidations that issues/recent_milestones/releases/top_contributors are typed as lists of expected node types, and that url/owner_key/latest_release are str, are appropriate.
Also applies to: 62-76, 77-91, 97-101
1-204: Verify internal node modules coverage (≥ 80%)The sandbox environment doesn’t include
pytest, so please run this locally to confirm that all files underbackend/apps/github/api/internal/nodesmeet the 80% coverage threshold:#!/bin/bash set -euo pipefail # Run tests for internal nodes with coverage pytest -q \ backend/tests/apps/github/api/internal/nodes \ --cov=backend/apps/github/api/internal/nodes \ --cov-report=term-missing # Enforce per-file threshold of 80% coverage xml -o coverage.xml python - <<'PY' import xml.etree.ElementTree as ET tree = ET.parse('coverage.xml') root = tree.getroot() fail = False print("Per-file coverage for backend/apps/github/api/internal/nodes:") for pkg in root.findall(".//packages/package"): for cls in pkg.findall("classes/class"): filename = cls.attrib["filename"] if filename.startswith("backend/apps/github/api/internal/nodes/") and filename.endswith(".py"): lines = cls.find("lines") covered = sum( 1 for l in lines for li in l.findall("line") if float(li.attrib.get("hits", "0")) > 0 ) total = sum(len(l.findall("line")) for l in lines) pct = (covered / total * 100) if total else 100.0 print(f" {filename}: {pct:.2f}%") if pct < 80.0: fail = True if fail: raise SystemExit("Coverage threshold not met (>=80% required).") print("All target files meet or exceed 80%.") PYPlease verify and share the coverage report.
| def test_project_name_with_project(self): | ||
| """Test project_name field when project exists.""" | ||
| mock_release = Mock() | ||
| mock_repository = Mock() | ||
| mock_project = Mock() | ||
| mock_project.name = "OWASP Test Project" | ||
| mock_repository.project = mock_project | ||
| mock_release.repository = mock_repository | ||
|
|
||
| result = ReleaseNode.project_name(mock_release) | ||
| assert result == " Test Project" # OWASP prefix stripped | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential semantic pitfall with lstrip when stripping “OWASP” prefix
The production code uses str.lstrip(OWASP_ORGANIZATION_NAME). lstrip removes any leading characters contained in the argument, not the exact prefix. This can unexpectedly trim names that merely start with any of the letters in “OWASP”. Consider switching the implementation to removeprefix for exact semantics, e.g., name.removeprefix(OWASP_ORGANIZATION_NAME) or name.removeprefix(f"{OWASP_ORGANIZATION_NAME} ") if a trailing space is intended.
If you’d like, I can raise a follow-up issue and propose a patch to replace lstrip with removeprefix and update tests accordingly.
🏁 Script executed:
#!/bin/bash
# Locate the implementation of ReleaseNode.project_name
rg -n "def project_name" -R backend
# Find where OWASP_ORGANIZATION_NAME is defined or used
rg -n "OWASP_ORGANIZATION_NAME" -R backendLength of output: 890
🏁 Script executed:
#!/bin/bash
# Locate the ReleaseNode class and its project_name method
rg -n "class ReleaseNode" backend
rg -n "def project_name" backend
# Inspect any usage of lstrip and the OWASP_ORGANIZATION_NAME constant
rg -n "lstrip" backend
rg -n "OWASP_ORGANIZATION_NAME" backendLength of output: 2106
🏁 Script executed:
#!/bin/bash
# Display the ReleaseNode implementation to inspect project_name
sed -n '1,100p' backend/apps/github/api/internal/nodes/release.pyLength of output: 1450
Use removeprefix for exact “OWASP” removal instead of lstrip
The project_name resolver currently does:
self.repository.project.name.lstrip(OWASP_ORGANIZATION_NAME)But str.lstrip(chars) strips any leading character found in chars, not the exact substring. That means names like "SOAP Project" or "PASSion Project" could get mangled.
Switch to str.removeprefix to remove only the literal "OWASP" (and its trailing space, if desired):
• backend/apps/github/api/internal/nodes/release.py
- return (
- self.repository.project.name.lstrip(OWASP_ORGANIZATION_NAME)
- if self.repository and self.repository.project
- else None
- )
+ if not (self.repository and self.repository.project):
+ return None
+ return self.repository.project.name.removeprefix(f"{OWASP_ORGANIZATION_NAME} ")• backend/tests/apps/github/api/internal/nodes/release_test.py
- result = ReleaseNode.project_name(mock_release)
- assert result == " Test Project" # OWASP prefix stripped
+ result = ReleaseNode.project_name(mock_release)
+ assert result == "Test Project" # "OWASP " prefix removed exactlyThese changes ensure only the exact "OWASP" (plus space) is stripped, preventing unintended trimming of other names.
trucodd
force-pushed
the
feature/github-api-nodes-cov
branch
from
f137e66 to
0b2432a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
backend/tests/apps/github/api/internal/nodes/repository_test.py (4)
14-16: Rename test to reflect its intent (not inheritance).The test asserts Strawberry type registration, not inheritance. Rename for clarity.
-def test_repository_node_inheritance(self): +def test_repository_node_is_strawberry_type(self): assert hasattr(RepositoryNode, "__strawberry_definition__")
17-45: Strengthen meta-configuration assertion to catch regressions.Use strict equality to surface unexpected additions/removals. Other node tests in this repo also use equality, so this aligns style and increases signal.
-assert expected_field_names.issubset(field_names) +assert field_names == expected_field_names
115-122: Avoid order assumptions when asserting languages.Dictionary key order is insertion-dependent. Asserting as a set makes the test robust.
result = RepositoryNode.languages(mock_repository) -assert result == ["Python", "JavaScript"] +assert set(result) == {"Python", "JavaScript"}
17-45: Optional: add quick type assertions for scalar fields to close gaps.For completeness and to bump coverage with minimal cost, consider adding type checks (like existing ones) for: key, name, description, license, commits_count, contributors_count, forks_count, open_issues_count, size, stars_count, subscribers_count, created_at, updated_at.
I can add a compact helper and parameterize assertions to keep the test concise.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/tests/apps/github/api/internal/nodes/issue_test.py(2 hunks)backend/tests/apps/github/api/internal/nodes/milestone_test.py(2 hunks)backend/tests/apps/github/api/internal/nodes/organization_test.py(3 hunks)backend/tests/apps/github/api/internal/nodes/pull_request_test.py(1 hunks)backend/tests/apps/github/api/internal/nodes/release_test.py(2 hunks)backend/tests/apps/github/api/internal/nodes/repository_test.py(1 hunks)backend/tests/apps/github/api/internal/nodes/user_test.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/tests/apps/github/api/internal/nodes/milestone_test.py
- backend/tests/apps/github/api/internal/nodes/issue_test.py
- backend/tests/apps/github/api/internal/nodes/release_test.py
- backend/tests/apps/github/api/internal/nodes/pull_request_test.py
- backend/tests/apps/github/api/internal/nodes/organization_test.py
- backend/tests/apps/github/api/internal/nodes/user_test.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/tests/apps/github/api/internal/nodes/repository_test.py (10)
backend/apps/github/api/internal/nodes/issue.py (1)
IssueNode(19-39)backend/apps/github/api/internal/nodes/milestone.py (1)
MilestoneNode(22-50)backend/apps/github/api/internal/nodes/organization.py (1)
OrganizationNode(39-76)backend/apps/github/api/internal/nodes/release.py (1)
ReleaseNode(20-54)backend/apps/github/api/internal/queries/repository.py (1)
repository(14-35)backend/apps/github/api/internal/nodes/repository.py (2)
RepositoryNode(40-100)owner_key(65-67)backend/apps/github/api/internal/nodes/repository_contributor.py (1)
RepositoryContributorNode(7-15)backend/tests/apps/github/api/internal/nodes/organization_test.py (3)
test_meta_configuration(15-32)test_resolve_url(42-47)test_url_method(117-123)backend/tests/apps/github/api/internal/nodes/release_test.py (1)
test_meta_configuration(15-28)backend/tests/apps/github/api/internal/queries/repository_test.py (1)
mock_repository(15-19)
🔇 Additional comments (1)
backend/tests/apps/github/api/internal/nodes/repository_test.py (1)
52-56: LGTM: Resolver and type introspection tests are on-point.Good coverage of field types and resolver behaviors with lightweight mocks. Suggested tweaks above will harden the tests further.
Also applies to: 62-76, 77-81, 82-86, 87-91, 92-96, 97-101, 123-156, 180-188, 189-196, 197-204
| from apps.github.api.internal.nodes.repository import RepositoryNode | ||
| from apps.owasp.api.internal.nodes.project import ProjectNode | ||
| from apps.github.api.internal.nodes.repository_contributor import RepositoryContributorNode |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import limits to assert slice sizes in resolver tests.
Asserting the slice size protects against silent regressions in pagination/limit behavior.
-from apps.github.api.internal.nodes.repository import RepositoryNode
+from apps.github.api.internal.nodes.repository import (
+ RepositoryNode,
+ RECENT_ISSUES_LIMIT,
+ RECENT_RELEASES_LIMIT,
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from apps.github.api.internal.nodes.repository import RepositoryNode | |
| from apps.owasp.api.internal.nodes.project import ProjectNode | |
| from apps.github.api.internal.nodes.repository_contributor import RepositoryContributorNode | |
| from apps.github.api.internal.nodes.repository import ( | |
| RepositoryNode, | |
| RECENT_ISSUES_LIMIT, | |
| RECENT_RELEASES_LIMIT, | |
| ) | |
| from apps.github.api.internal.nodes.repository_contributor import RepositoryContributorNode |
🤖 Prompt for AI Agents
In backend/tests/apps/github/api/internal/nodes/repository_test.py around lines
9-10, add an import for the resolver's pagination limit constant (the
module/file where the resolver defines its default limit) and use that constant
in the test assertions that check slice sizes so the tests assert against the
actual configured limit (e.g. replace hardcoded numbers with the imported
LIMIT/PAGE_SIZE constant), ensuring future changes to the resolver limit are
reflected in tests.
| def test_issues_method(self): | ||
| """Test issues method resolution.""" | ||
| mock_repository = Mock() | ||
| mock_issues = Mock() | ||
| mock_issues.select_related.return_value.order_by.return_value.__getitem__ = Mock( | ||
| return_value=[] | ||
| ) | ||
| mock_repository.issues = mock_issues | ||
|
|
||
| RepositoryNode.issues(mock_repository) | ||
| mock_issues.select_related.assert_called_with("author") | ||
| mock_issues.select_related.return_value.order_by.assert_called_with("-created_at") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also assert the issues slice limit.
Verifies [:RECENT_ISSUES_LIMIT] is applied, not just the queryset chaining.
RepositoryNode.issues(mock_repository)
mock_issues.select_related.assert_called_with("author")
mock_issues.select_related.return_value.order_by.assert_called_with("-created_at")
+mock_issues.select_related.return_value.order_by.return_value.__getitem__.assert_called_with(
+ slice(None, RECENT_ISSUES_LIMIT)
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_issues_method(self): | |
| """Test issues method resolution.""" | |
| mock_repository = Mock() | |
| mock_issues = Mock() | |
| mock_issues.select_related.return_value.order_by.return_value.__getitem__ = Mock( | |
| return_value=[] | |
| ) | |
| mock_repository.issues = mock_issues | |
| RepositoryNode.issues(mock_repository) | |
| mock_issues.select_related.assert_called_with("author") | |
| mock_issues.select_related.return_value.order_by.assert_called_with("-created_at") | |
| def test_issues_method(self): | |
| """Test issues method resolution.""" | |
| mock_repository = Mock() | |
| mock_issues = Mock() | |
| mock_issues.select_related.return_value.order_by.return_value.__getitem__ = Mock( | |
| return_value=[] | |
| ) | |
| mock_repository.issues = mock_issues | |
| RepositoryNode.issues(mock_repository) | |
| mock_issues.select_related.assert_called_with("author") | |
| mock_issues.select_related.return_value.order_by.assert_called_with("-created_at") | |
| + mock_issues.select_related.return_value.order_by.return_value.__getitem__.assert_called_with( | |
| + slice(None, RECENT_ISSUES_LIMIT) | |
| + ) |
🤖 Prompt for AI Agents
In backend/tests/apps/github/api/internal/nodes/repository_test.py around lines
102 to 114, add an assertion that the issues queryset slicing is applied: after
calling RepositoryNode.issues(mock_repository), assert that
mock_issues.select_related.return_value.order_by.return_value.__getitem__ was
called with the slice corresponding to the RECENT_ISSUES_LIMIT (e.g. slice(None,
RepositoryNode.RECENT_ISSUES_LIMIT, None)) so the test verifies the
[:RECENT_ISSUES_LIMIT] limit is enforced.
| def test_recent_milestones_method(self): | ||
| """Test recent_milestones method resolution.""" | ||
| mock_repository = Mock() | ||
| mock_milestones = Mock() | ||
| mock_milestones.select_related.return_value.order_by.return_value.__getitem__ = Mock( | ||
| return_value=[] | ||
| ) | ||
| mock_repository.recent_milestones = mock_milestones | ||
|
|
||
| RepositoryNode.recent_milestones(mock_repository, limit=3) | ||
| mock_milestones.select_related.assert_called_with("repository") | ||
| mock_milestones.select_related.return_value.order_by.assert_called_with("-created_at") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assert recent_milestones respects the passed limit.
Confirms slicing is applied with the provided limit.
RepositoryNode.recent_milestones(mock_repository, limit=3)
mock_milestones.select_related.assert_called_with("repository")
mock_milestones.select_related.return_value.order_by.assert_called_with("-created_at")
+mock_milestones.select_related.return_value.order_by.return_value.__getitem__.assert_called_with(
+ slice(None, 3)
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_recent_milestones_method(self): | |
| """Test recent_milestones method resolution.""" | |
| mock_repository = Mock() | |
| mock_milestones = Mock() | |
| mock_milestones.select_related.return_value.order_by.return_value.__getitem__ = Mock( | |
| return_value=[] | |
| ) | |
| mock_repository.recent_milestones = mock_milestones | |
| RepositoryNode.recent_milestones(mock_repository, limit=3) | |
| mock_milestones.select_related.assert_called_with("repository") | |
| mock_milestones.select_related.return_value.order_by.assert_called_with("-created_at") | |
| def test_recent_milestones_method(self): | |
| """Test recent_milestones method resolution.""" | |
| mock_repository = Mock() | |
| mock_milestones = Mock() | |
| mock_milestones.select_related.return_value.order_by.return_value.__getitem__ = Mock( | |
| return_value=[] | |
| ) | |
| mock_repository.recent_milestones = mock_milestones | |
| RepositoryNode.recent_milestones(mock_repository, limit=3) | |
| mock_milestones.select_related.assert_called_with("repository") | |
| mock_milestones.select_related.return_value.order_by.assert_called_with("-created_at") | |
| mock_milestones.select_related.return_value.order_by.return_value.__getitem__.assert_called_with( | |
| slice(None, 3) | |
| ) |
🤖 Prompt for AI Agents
In backend/tests/apps/github/api/internal/nodes/repository_test.py around lines
157 to 169, the test checks select_related and order_by but does not assert that
the returned queryset is sliced using the provided limit; add an assertion that
the mocked queryset's __getitem__ was called with a slice for the given limit
(e.g., slice(None, 3)) to confirm slicing was applied when calling
RepositoryNode.recent_milestones(mock_repository, limit=3).
| def test_releases_method(self): | ||
| """Test releases method resolution.""" | ||
| mock_repository = Mock() | ||
| mock_releases = Mock() | ||
| mock_releases.order_by.return_value.__getitem__ = Mock(return_value=[]) | ||
| mock_repository.published_releases = mock_releases | ||
|
|
||
| RepositoryNode.releases(mock_repository) | ||
| mock_releases.order_by.assert_called_with("-published_at") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also assert the releases slice limit.
Covers [:RECENT_RELEASES_LIMIT] to guard limit regressions.
RepositoryNode.releases(mock_repository)
mock_releases.order_by.assert_called_with("-published_at")
+mock_releases.order_by.return_value.__getitem__.assert_called_with(
+ slice(None, RECENT_RELEASES_LIMIT)
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_releases_method(self): | |
| """Test releases method resolution.""" | |
| mock_repository = Mock() | |
| mock_releases = Mock() | |
| mock_releases.order_by.return_value.__getitem__ = Mock(return_value=[]) | |
| mock_repository.published_releases = mock_releases | |
| RepositoryNode.releases(mock_repository) | |
| mock_releases.order_by.assert_called_with("-published_at") | |
| def test_releases_method(self): | |
| """Test releases method resolution.""" | |
| mock_repository = Mock() | |
| mock_releases = Mock() | |
| mock_releases.order_by.return_value.__getitem__ = Mock(return_value=[]) | |
| mock_repository.published_releases = mock_releases | |
| RepositoryNode.releases(mock_repository) | |
| mock_releases.order_by.assert_called_with("-published_at") | |
| mock_releases.order_by.return_value.__getitem__.assert_called_with( | |
| slice(None, RECENT_RELEASES_LIMIT) | |
| ) |
🤖 Prompt for AI Agents
In backend/tests/apps/github/api/internal/nodes/repository_test.py around lines
170 to 179, add an assertion that the releases slice limit is applied: after
asserting mock_releases.order_by was called with "-published_at", assert that
mock_releases.order_by.return_value.__getitem__ was called with the slice up to
the RECENT_RELEASES_LIMIT (i.e. slice(None,
RepositoryNode.RECENT_RELEASES_LIMIT, None) or equivalent
[:RECENT_RELEASES_LIMIT]); this ensures the test covers the limit and prevents
regressions.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
backend/tests/apps/github/api/internal/nodes/repository_test.py (4)
9-11: Import resolver limit constants to assert slice sizes explicitly.Brings the test in sync with the implementation and prevents regressions if limits change.
-from apps.github.api.internal.nodes.repository import RepositoryNode +from apps.github.api.internal.nodes.repository import ( + RepositoryNode, + RECENT_ISSUES_LIMIT, + RECENT_RELEASES_LIMIT, +)
103-115: Also assert issues slice limit is applied.Ensures [:RECENT_ISSUES_LIMIT] is enforced, not just the queryset chaining.
RepositoryNode.issues(mock_repository) mock_issues.select_related.assert_called_with("author") mock_issues.select_related.return_value.order_by.assert_called_with("-created_at") + mock_issues.select_related.return_value.order_by.return_value.__getitem__.assert_called_with( + slice(None, RECENT_ISSUES_LIMIT) + )
171-180: Also assert releases slice limit is applied.Covers [:RECENT_RELEASES_LIMIT] to guard limit regressions.
RepositoryNode.releases(mock_repository) mock_releases.order_by.assert_called_with("-published_at") + mock_releases.order_by.return_value.__getitem__.assert_called_with( + slice(None, RECENT_RELEASES_LIMIT) + )
158-170: Assert recent_milestones respects the provided limit.Confirms slicing is applied with the passed limit.
RepositoryNode.recent_milestones(mock_repository, limit=3) mock_milestones.select_related.assert_called_with("repository") mock_milestones.select_related.return_value.order_by.assert_called_with("-created_at") + mock_milestones.select_related.return_value.order_by.return_value.__getitem__.assert_called_with( + slice(None, 3) + )
🧹 Nitpick comments (8)
backend/tests/apps/github/api/internal/nodes/milestone_test.py (2)
16-32: Make field assertions resilient to future schema growth (use subset).Asserting exact equality on field names is brittle. Prefer subset to guard against harmless additions.
- assert field_names == expected_field_names + assert expected_field_names.issubset(field_names)
73-90: Add a rounding edge-case to lock the 2-decimal behavior.Progress is rounded to 2 decimals in production. Add a fractional case to prevent regressions in rounding precision.
@@ def test_progress_without_issues(self): """Test progress calculation without issues.""" mock_milestone = Mock() mock_milestone.closed_issues_count = 0 mock_milestone.open_issues_count = 0 result = MilestoneNode.progress(mock_milestone) assert result == pytest.approx(0.0) + + def test_progress_rounding_two_decimals(self): + """Test progress rounding to two decimals.""" + mock_milestone = Mock() + mock_milestone.closed_issues_count = 1 + mock_milestone.open_issues_count = 6 # 1 / 7 = 14.2857... + + result = MilestoneNode.progress(mock_milestone) + assert result == pytest.approx(14.29)backend/tests/apps/github/api/internal/nodes/organization_test.py (5)
15-33: Prefer subset check for field names to avoid brittle failures.Exact equality will fail if harmless fields are added. Using subset balances strictness and flexibility.
- assert field_names == expected_field_names + assert expected_field_names.issubset(field_names)
50-52: Patch where the symbol is looked up (module-under-test) to reduce coupling.Patching the model class attribute works, but patching the names as imported in the module under test is usually safer against import refactors.
- @patch("apps.github.models.repository.Repository.objects") - @patch("apps.github.models.repository_contributor.RepositoryContributor.objects") + @patch("apps.github.api.internal.nodes.organization.Repository.objects") + @patch("apps.github.api.internal.nodes.organization.RepositoryContributor.objects")
83-85: Apply the same patch-target refinement here.- @patch("apps.github.models.repository.Repository.objects") - @patch("apps.github.models.repository_contributor.RepositoryContributor.objects") + @patch("apps.github.api.internal.nodes.organization.Repository.objects") + @patch("apps.github.api.internal.nodes.organization.RepositoryContributor.objects")
69-82: Also assert repository and contributor queryset filters were invoked with expected args.Strengthens the test by verifying the key query filters.
# Call stats method result = OrganizationNode.stats(mock_organization) # Verify result assert isinstance(result, OrganizationStatsNode) assert result.total_repositories == 5 assert result.total_contributors == 15 assert result.total_stars == 100 assert result.total_forks == 50 assert result.total_issues == 25 + # Verify filtering arguments + mock_repository_objects.filter.assert_called_with(organization=mock_organization) + mock_repo_contributor_objects.filter.assert_called_with( + repository__in=mock_repositories + )
107-117: Mirror filter assertions in the None-aggregates case as well.Ensures we validate query shapes in both branches.
# Verify result with default values assert isinstance(result, OrganizationStatsNode) assert result.total_repositories == 3 assert result.total_contributors == 8 assert result.total_stars == 0 assert result.total_forks == 0 assert result.total_issues == 0 + mock_repository_objects.filter.assert_called_with(organization=mock_organization) + mock_repo_contributor_objects.filter.assert_called_with( + repository__in=mock_repositories + )backend/tests/apps/github/api/internal/nodes/repository_test.py (1)
116-123: Avoid order brittleness in languages; assert as a set (or sort).Ordering can change if the implementation sorts by bytes or key; assert membership instead.
- result = RepositoryNode.languages(mock_repository) - assert result == ["Python", "JavaScript"] + result = RepositoryNode.languages(mock_repository) + assert set(result) == {"Python", "JavaScript"}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
backend/tests/apps/github/api/internal/nodes/issue_test.py(2 hunks)backend/tests/apps/github/api/internal/nodes/milestone_test.py(2 hunks)backend/tests/apps/github/api/internal/nodes/organization_test.py(3 hunks)backend/tests/apps/github/api/internal/nodes/pull_request_test.py(1 hunks)backend/tests/apps/github/api/internal/nodes/release_test.py(2 hunks)backend/tests/apps/github/api/internal/nodes/repository_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/tests/apps/github/api/internal/nodes/issue_test.py
- backend/tests/apps/github/api/internal/nodes/release_test.py
- backend/tests/apps/github/api/internal/nodes/pull_request_test.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/tests/apps/github/api/internal/nodes/milestone_test.py (2)
backend/apps/github/api/internal/nodes/milestone.py (5)
author(26-28)MilestoneNode(22-50)organization_name(31-37)progress(40-45)repository_name(48-50)backend/apps/github/api/internal/nodes/pull_request.py (3)
author(21-23)organization_name(26-32)repository_name(35-37)
backend/tests/apps/github/api/internal/nodes/organization_test.py (1)
backend/apps/github/api/internal/nodes/organization.py (3)
stats(43-71)OrganizationStatsNode(13-20)url(74-76)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (6)
backend/tests/apps/github/api/internal/nodes/milestone_test.py (2)
34-42: LGTM: direct resolver call pattern is clear and correct.Good use of an instance mock to exercise the resolver without constructing the whole node.
43-54: LGTM: org login resolution path is covered.Covers repository.organization.login happy path accurately.
backend/tests/apps/github/api/internal/nodes/organization_test.py (3)
35-49: LGTM: schema-level type checks for stats and url fields.Good sanity checks on field presence and declared types.
118-125: LGTM: url resolver directly returns backing value.
141-155: LGTM: OrganizationStatsNode construction assertions are clear.backend/tests/apps/github/api/internal/nodes/repository_test.py (1)
98-101: LGTM: url field type check is appropriate.
OWASP#2042) * test- inmproved test coverage of github internal nodes more than 80% updated * Update code --------- Co-authored-by: Arkadii Yakovets <[email protected]>
2 participants
…updated
Proposed change
Resolves #1797
Improved test coverage
Checklist
make check-testlocally; all checks and tests passed.