feat: improve backend test coverage to 100%#3991
Merged
arkid15r merged 9 commits intoOWASP:mainfrom Feb 18, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds many new unit tests across OWASP, GitHub, Slack, and API modules; initializes a YAML metadata dict before parsing in owasp_sync_posts; and simplifies leader-email extraction logic in get_leaders_emails. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (14)
backend/tests/apps/owasp/index/search/chapter_test.py (1)
41-42: Optional: prefercall_args.argsovercall_args[0]for positional-arg inspection.
call_args[0](the legacy subscript) andcall_args.args(Python 3.8+ named attribute) are functionally identical here, butcall_args.argsis more self-documenting and avoids the "magic index" feel. The same pattern applies to the identical blocks inissue_test.pyandproject_test.py.♻️ Proposed refactor
- call_args = mock_raw_search.call_args[0] - params = call_args[2] + params = mock_raw_search.call_args.args[2]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/index/search/chapter_test.py` around lines 41 - 42, Replace the legacy positional-access pattern using mock_raw_search.call_args[0] with the more explicit mock_raw_search.call_args and its .args attribute: assign call_args = mock_raw_search.call_args and then use params = call_args.args[2] (instead of call_args[2]); apply the same change in the analogous blocks in issue_test.py and project_test.py so positional-arg inspection uses call_args.args throughout.backend/tests/apps/slack/utils_test.py (3)
223-223: Trailing underscore in test function nametest_get_gsoc_projects_.The extra
_suffix is inconsistent with all other test function names in this file.♻️ Proposed fix
-def test_get_gsoc_projects_(monkeypatch): +def test_get_gsoc_projects(monkeypatch):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/slack/utils_test.py` at line 223, The test function is misnamed with a trailing underscore: rename test_get_gsoc_projects_ to test_get_gsoc_projects to match the naming convention; update any references or test discovery expectations that call test_get_gsoc_projects_ (e.g., in the file backend/tests/apps/slack/utils_test.py) so they point to the new name and run the tests to ensure no references remain.
334-345:assert result is not Noneis a trivially weak assertion.Both
test_get_sponsors_dataandtest_get_posts_dataset up__getitem__on a mock queryset but never check whatresultactually contains — the assertion passes even if the function returns any truthy object. Consider asserting on the returned slice or at minimum verifying__getitem__was invoked with the expected argument.♻️ Proposed fix
# test_get_sponsors_data result = get_sponsors_data(limit=5) mock_objects.all.assert_called_once() - assert result is not None + mock_queryset.__getitem__.assert_called_once_with(slice(None, 5)) # test_get_posts_data result = get_posts_data(limit=3) mock_recent.assert_called_once() - assert result is not None + mock_queryset.__getitem__.assert_called_once_with(slice(None, 3))Also applies to: 357-369
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/slack/utils_test.py` around lines 334 - 345, The test uses a mock queryset with __getitem__ but only asserts result is not None; update test_get_sponsors_data to assert that mock_queryset.__getitem__ was called with the expected slice (e.g. slice(0, 5) or equivalent) and that the returned value equals the mocked slice value (mock_sponsor list) from get_sponsors_data; locate the test and its mock setup (mock_queryset and the patch of apps.owasp.models.sponsor.Sponsor.objects) and add assertions for mock_queryset.__getitem__.assert_called_once_with(slice(0, 5)) and assert result == [mock_sponsor] (or the exact mocked return) to validate slicing behavior of get_sponsors_data.
252-276:lengthvariable is reused for unrelated semantic purposes, masking potential assertion bugs.
lengthis first set to3(expected news-item count), reassigned to2for thelimitargument, and then the samelengthvariable is used as the expectedmock_get.call_count. The assertion passes by coincidence — if the call count or the limit ever change independently, the assertion will silently test the wrong thing.♻️ Proposed fix
result = get_news_data() - length = 3 - assert len(result) == length + assert len(result) == 3 assert result[0]["title"] == "News Title 1" assert result[0]["author"] == "Author 1" assert result[0]["url"] == urljoin(OWASP_NEWS_URL, "/news1") - length = 2 - result_limited = get_news_data(limit=2) - assert len(result_limited) == length + result_limited = get_news_data(limit=2) + assert len(result_limited) == 2 mock_get.assert_called_with(OWASP_NEWS_URL, timeout=30) result2 = get_news_data() - assert mock_get.call_count == length + assert mock_get.call_count == 2 # third call uses cache assert result == result2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/slack/utils_test.py` around lines 252 - 276, The test reuses the variable name length for three different meanings which masks bugs; update test_get_news_data to use distinct, explicit variables (e.g., expected_total_items = 3, expected_limited_items = 2, expected_get_calls = 2) and change assertions to use those names: assert len(result) == expected_total_items, assert len(result_limited) == expected_limited_items, and assert mock_get.call_count == expected_get_calls; keep references to mock_get, get_news_data, and OWASP_NEWS_URL unchanged.backend/tests/apps/owasp/api/internal/queries/sponsor_test.py (2)
21-21: Redundantlist()call —sponsors()already returns a list.
sorted()returns alist, solist(query.sponsors())just copies it unnecessarily.♻️ Proposed cleanup
- result = list(query.sponsors()) + result = query.sponsors()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/api/internal/queries/sponsor_test.py` at line 21, The code creates an unnecessary copy by calling list() on the result of query.sponsors(); remove the redundant list() wrapper and assign result = query.sponsors() (or use result = sorted(query.sponsors()) only where sorting is needed) so the test uses the original list returned by the sponsors() method without an extra allocation; locate the usage in the test where result is assigned from query.sponsors() and update that assignment accordingly.
8-25: Add test cases for the remaining sponsor types (GOLD, SUPPORTER, NOT_SPONSOR).The test only exercises 3 of the 6
SponsorTypevalues (DIAMOND=1, PLATINUM=2, SILVER=4). The priority mappings forGOLD(3),SUPPORTER(5), andNOT_SPONSOR(6) are never validated. If those dictionary entries insponsors()were wrong or accidentally swapped, this test would not catch it.♻️ Suggested extension
class TestSponsorQuery: def test_sponsors_returns_sorted_by_type(self): """Test sponsors resolver sorts by sponsor type priority.""" diamond = MagicMock() diamond.sponsor_type = Sponsor.SponsorType.DIAMOND silver = MagicMock() silver.sponsor_type = Sponsor.SponsorType.SILVER platinum = MagicMock() platinum.sponsor_type = Sponsor.SponsorType.PLATINUM + gold = MagicMock() + gold.sponsor_type = Sponsor.SponsorType.GOLD + + supporter = MagicMock() + supporter.sponsor_type = Sponsor.SponsorType.SUPPORTER + + not_sponsor = MagicMock() + not_sponsor.sponsor_type = Sponsor.SponsorType.NOT_SPONSOR + - with patch.object(Sponsor.objects, "all", return_value=[silver, diamond, platinum]): + with patch.object( + Sponsor.objects, + "all", + return_value=[silver, not_sponsor, gold, diamond, supporter, platinum], + ): query = SponsorQuery() result = list(query.sponsors()) assert result[0] == diamond assert result[1] == platinum - assert result[2] == silver + assert result[2] == gold + assert result[3] == silver + assert result[4] == supporter + assert result[5] == not_sponsor🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/api/internal/queries/sponsor_test.py` around lines 8 - 25, Extend test_sponsors_returns_sorted_by_type to include MagicMock instances for Sponsor.SponsorType.GOLD, Sponsor.SponsorType.SUPPORTER, and Sponsor.SponsorType.NOT_SPONSOR, set their sponsor_type attributes accordingly, include them in the patched Sponsor.objects.all return_value alongside the existing diamond/platinum/silver in a mixed order, then call SponsorQuery().sponsors() and assert the full result list order matches the expected priority (DIAMOND, PLATINUM, GOLD, SILVER, SUPPORTER, NOT_SPONSOR) to validate the complete priority mapping in SponsorQuery.sponsors().backend/tests/apps/owasp/management/commands/common/entity_metadata_test.py (1)
100-103: Prefer a real instance overNoneas theselfargument.Passing
Noneasselfworks only because the current body never dereferencesself. If the base implementation is later extended to call any method onself, this test will raise anAttributeErrorat runtime without any obvious connection to the change that broke it. Use an instantiable concrete subclass already present in the file:♻️ Suggested fix
- result = EntityMetadataBase.get_metadata(None, entity=MagicMock()) + result = EntityMetadataBase.get_metadata(ConcreteTestCommand(), entity=MagicMock())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/management/commands/common/entity_metadata_test.py` around lines 100 - 103, The test test_get_metadata_abstract_returns_empty_dict should not pass None as the self arg to EntityMetadataBase.get_metadata; instead instantiate and use an existing concrete, instantiable subclass defined in the test file (replace the None in the call EntityMetadataBase.get_metadata(None, entity=MagicMock()) with an instance of that concrete subclass), so the test uses a real object and will not break if the base implementation later calls methods on self.backend/tests/apps/common/management/commands/add_project_custom_tags_test.py (1)
13-19: Consider adding an inline comment explaining thegetattrusage.
getattr(args, "file-name")is correct — argparse stores positional argument values under their literal dest string, and sinceargs.file-nameis a syntax error (parsed as subtraction),getattris required. However, this is non-obvious to a reader; a short inline comment would prevent future confusion.✨ Optional clarification comment
- assert getattr(args, "file-name") == "test-file.json" + # `args.file-name` is a syntax error; getattr is required for hyphenated dest names. + assert getattr(args, "file-name") == "test-file.json"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/common/management/commands/add_project_custom_tags_test.py` around lines 13 - 19, The test test_add_arguments in the Command class uses getattr(args, "file-name") which looks odd because args.file-name is a syntax error; add a brief inline comment next to that assertion explaining that argparse stores positional arguments under their literal dest (including hyphens) and therefore getattr is required to access the "file-name" dest—reference the add_arguments method/Command class and the args variable in the comment for clarity.backend/tests/apps/owasp/management/commands/owasp_create_member_snapshot_test.py (2)
514-515: Misleading test name — rename to match the actual scenario.The method is named
test_generate_heatmap_data_empty_issuesbut it passes a non-empty issue list with an out-of-range date; the[]in the call is the PR list. The docstring correctly says "issue outside date range". The name should reflect that.✏️ Suggested rename
- def test_generate_heatmap_data_empty_issues(self, command): - """Test generate_heatmap_data with issue outside date range.""" + def test_generate_heatmap_data_issue_out_of_range(self, command): + """Test generate_heatmap_data filters out issue outside date range."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/management/commands/owasp_create_member_snapshot_test.py` around lines 514 - 515, Rename the test method test_generate_heatmap_data_empty_issues to clearly reflect that it tests an issue outside the date range (e.g., test_generate_heatmap_data_issue_outside_date_range); update the method name and any references to it so the test name matches the docstring "issue outside date range" and adjust any test discovery references if present in tests or CI.
962-975: Missinggenerate_channel_communicationspatch — inconsistent with parallel test.
test_handle_with_slack_messages(lines 843–845) explicitly mocksgenerate_channel_communications, buttest_handle_with_slack_zero_messagesdoes not. If the production code invokesgenerate_channel_communicationsregardless of message count, the real implementation runs here on aMagicMockqueryset (which iterates to empty by default). While functionally harmless today, it silently exercises real code paths in a unit test and will break if the production implementation changes. The missing explicit__iter__setup (present intest_handle_with_slack_messagesat line 833) adds to the inconsistency.✏️ Suggested addition for consistency and isolation
mock_msg_qs = mock.MagicMock() mock_msg_qs.select_related.return_value = mock_msg_qs mock_msg_qs.count.return_value = 0 + mock_msg_qs.__iter__ = lambda _: iter([]) mock_message.objects.filter.return_value = mock_msg_qs mock_message.objects.none.return_value = mock_msg_qs mocker.patch.object(command, "generate_heatmap_data", return_value={}) mocker.patch.object(command, "generate_entity_contributions", return_value={}) mocker.patch.object(command, "generate_repository_contributions", return_value={}) mocker.patch.object(command, "generate_communication_heatmap_data", return_value={}) + mocker.patch.object(command, "generate_channel_communications", return_value={})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/management/commands/owasp_create_member_snapshot_test.py` around lines 962 - 975, In test_handle_with_slack_zero_messages, mock out generate_channel_communications and ensure the mocked queryset iteration is explicit: patch command.generate_channel_communications to return {} (like the other generators), and set mock_msg_qs.__iter__.return_value = iter([]) on the MagicMock (the same pattern used in test_handle_with_slack_messages) so the real implementation isn't exercised when calling command.handle(username="testuser", ...); keep references to mock_message, mock_msg_qs, command.generate_channel_communications, command.handle, and mock_snapshot.messages.add to locate where to apply the changes.backend/tests/apps/slack/management/commands/slack_sync_messages_test.py (1)
951-962: Repeated rate-limit error construction — consider a shared fixtureThe
SlackApiErrorsetup block (constructing aMagicMockresponse with__getitem__,.get, and.headers) is duplicated identically in bothTestFetchRepliesRateLimitmethods and in several earlier test classes. A class- or module-level pytest fixture would keep these DRY.♻️ Example fixture
`@pytest.fixture` def rate_limit_error(): error = SlackApiError( response={"ok": False, "error": "ratelimited"}, message="Rate limited", ) error.response = MagicMock() error.response.__getitem__ = lambda _self, key: ( "ratelimited" if key == "error" else None ) error.response.get = lambda key, default=None: ( "ratelimited" if key == "error" else default ) error.response.headers = {"Retry-After": "1"} return errorAlso applies to: 996-1007
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/slack/management/commands/slack_sync_messages_test.py` around lines 951 - 962, Extract the duplicated SlackApiError construction (the block creating SlackApiError, assigning error.response = MagicMock, setting response.__getitem__, response.get, and response.headers) into a reusable pytest fixture (e.g., rate_limit_error) at module scope (or in conftest.py) and update the TestFetchRepliesRateLimit test methods (and other tests around the duplicated blocks) to accept that fixture instead of rebuilding the mock; locate occurrences by the SlackApiError instantiation and the MagicMock response setup and replace them with the fixture injection.backend/tests/apps/owasp/management/commands/owasp_update_leaders_test.py (1)
248-249:Userpatch is unreachable in this code path.
handle()returns early at themodel_name not in model_mapguard before it ever reachesUser.objects.values(...), so thewith patch(...)context manager here is dead code and may mislead future readers.🧹 Suggested cleanup
- with patch(f"{COMMAND_PATH}.User"): - cmd.handle(model_name="invalid", threshold=75) + cmd.handle(model_name="invalid", threshold=75)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/management/commands/owasp_update_leaders_test.py` around lines 248 - 249, The test currently patches User inside a call to cmd.handle(model_name="invalid", threshold=75) but cmd.handle returns early due to the model_name not in model_map guard, making the with patch(f"{COMMAND_PATH}.User") unreachable; remove the dead with patch(...) block (or move/replace it into a separate test that calls cmd.handle with a valid model_name so User.objects.values(...) is exercised) and keep the assertion(s) that verify the early-return behavior of cmd.handle(model_name="invalid") instead.backend/tests/apps/owasp/management/commands/owasp_sync_posts_test.py (1)
218-239: Core assertions are correct; consider adding astderrassertion for completeness.The test correctly exercises the no-YAML-match path and verifies
update_datais never called andbulk_savereceives an empty list. The parallel testtest_handle_missing_required_fields(line 216) additionally assertscommand.stderr.write.assert_called()since both paths go through the same warning branch. Adding the same assertion here would align the two tests and explicitly confirm the warning is emitted.♻️ Suggested addition
assert mock_get_content.call_count == 2 mock_update_data.assert_not_called() mock_bulk_save.assert_called_once_with([]) + command.stderr.write.assert_called()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/management/commands/owasp_sync_posts_test.py` around lines 218 - 239, Add an assertion that the command wrote a warning to stderr in the no-YAML-match test: after calling command.handle() in test_handle_no_yaml_match, assert that command.stderr.write.assert_called() to mirror test_handle_missing_required_fields and confirm the warning branch was exercised; keep existing assertions for mock_get_content.call_count, mock_update_data.assert_not_called(), and mock_bulk_save.assert_called_once_with([]).backend/tests/apps/owasp/management/commands/owasp_aggregate_entity_contributions_test.py (1)
460-462:assert_called()is weaker than the parallel test uses — considerassert_called_once().
test_handle_with_offset(line 427) usesmock_aggregate.assert_called_once()for the same conceptual check.assert_called()on line 462 would pass even ifaggregate_contributionswere called multiple times, reducing diagnostic value.♻️ Proposed fix
- mock_aggregate.assert_called() + mock_aggregate.assert_called_once()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/management/commands/owasp_aggregate_entity_contributions_test.py` around lines 460 - 462, Replace the weak mock assertion with a stronger one: change the call to mock_aggregate.assert_called() in the test (the assertion verifying aggregate_contributions was invoked) to mock_aggregate.assert_called_once() so the test ensures aggregate_contributions was invoked exactly once (consistent with the other test test_handle_with_offset that uses assert_called_once()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/owasp/models/common.py`:
- Around line 240-242: get_leaders_emails is returning arbitrary markdown link
URLs (e.g. https://...) as member_email which later causes Django EmailField
validation errors in sync_leaders/EntityMember.update_data; change the
extraction to only capture valid email addresses or mailto: links and normalize
them to plain emails (e.g. extract after "mailto:") and return an empty string
for non-email links so sync_leaders doesn't set invalid emails, or alternatively
decide to store arbitrary contact URLs by switching member_email to a
CharField—update get_leaders_emails (and the regex it uses) to enforce the
chosen behavior.
In `@backend/tests/apps/owasp/index/registry/chapter_test.py`:
- Line 9: The patch.object call uses a lambda with parameter name self which is
unused, causing Ruff ARG005; update the lambda to use a throwaway parameter name
(e.g., replace "lambda self: None" with "lambda _: None") in the patch for
ChapterIndex.__init__ so the signature still accepts the instance but the linter
is satisfied.
In `@backend/tests/apps/owasp/index/registry/committee_test.py`:
- Line 9: Replace the unused-argument lambda in the patch call so it doesn't
trigger Ruff ARG005: change the current patch.object(CommitteeIndex, "__init__",
lambda self: None) to use a no-op that accepts any args, e.g.
patch.object(CommitteeIndex, "__init__", lambda *args, **kwargs: None), keeping
the target class/method name CommitteeIndex.__init__ unchanged.
In `@backend/tests/apps/slack/apps_test.py`:
- Line 68: The test unpacking of _reload_with_mock_app() assigns an unused
variable named module; update both occurrences in
test_log_events_creates_event_and_calls_next and
test_log_events_handles_exception so the unpacking uses _module (or simply _ )
instead of module (e.g., change "handlers, module =
self._reload_with_mock_app()" to "handlers, _module =
self._reload_with_mock_app()" or "handlers, _ = ...") to satisfy Ruff RUF059 and
make the intent explicit.
In `@backend/tests/apps/slack/management/commands/slack_sync_messages_test.py`:
- Around line 757-789: The test currently only runs Command._sync_user_messages
but makes no assertions that messages missing a channel were skipped; update the
test_sync_message_no_channel_id to assert that
Conversation.objects.get_or_create (or Conversation.objects.get_or_create mocked
via Workspace/Conversation) was never called and that Command._create_message
(or whatever internal method creates messages) was not invoked for the match
with an empty channel, by patching/mocking Conversation.objects.get_or_create
and Command._create_message (or _create_message) and adding
assert_not_called/assert_called_with checks to confirm no conversation or
message creation occurred for the match with channel == {}.
---
Nitpick comments:
In
`@backend/tests/apps/common/management/commands/add_project_custom_tags_test.py`:
- Around line 13-19: The test test_add_arguments in the Command class uses
getattr(args, "file-name") which looks odd because args.file-name is a syntax
error; add a brief inline comment next to that assertion explaining that
argparse stores positional arguments under their literal dest (including
hyphens) and therefore getattr is required to access the "file-name"
dest—reference the add_arguments method/Command class and the args variable in
the comment for clarity.
In `@backend/tests/apps/owasp/api/internal/queries/sponsor_test.py`:
- Line 21: The code creates an unnecessary copy by calling list() on the result
of query.sponsors(); remove the redundant list() wrapper and assign result =
query.sponsors() (or use result = sorted(query.sponsors()) only where sorting is
needed) so the test uses the original list returned by the sponsors() method
without an extra allocation; locate the usage in the test where result is
assigned from query.sponsors() and update that assignment accordingly.
- Around line 8-25: Extend test_sponsors_returns_sorted_by_type to include
MagicMock instances for Sponsor.SponsorType.GOLD, Sponsor.SponsorType.SUPPORTER,
and Sponsor.SponsorType.NOT_SPONSOR, set their sponsor_type attributes
accordingly, include them in the patched Sponsor.objects.all return_value
alongside the existing diamond/platinum/silver in a mixed order, then call
SponsorQuery().sponsors() and assert the full result list order matches the
expected priority (DIAMOND, PLATINUM, GOLD, SILVER, SUPPORTER, NOT_SPONSOR) to
validate the complete priority mapping in SponsorQuery.sponsors().
In `@backend/tests/apps/owasp/index/search/chapter_test.py`:
- Around line 41-42: Replace the legacy positional-access pattern using
mock_raw_search.call_args[0] with the more explicit mock_raw_search.call_args
and its .args attribute: assign call_args = mock_raw_search.call_args and then
use params = call_args.args[2] (instead of call_args[2]); apply the same change
in the analogous blocks in issue_test.py and project_test.py so positional-arg
inspection uses call_args.args throughout.
In `@backend/tests/apps/owasp/management/commands/common/entity_metadata_test.py`:
- Around line 100-103: The test test_get_metadata_abstract_returns_empty_dict
should not pass None as the self arg to EntityMetadataBase.get_metadata; instead
instantiate and use an existing concrete, instantiable subclass defined in the
test file (replace the None in the call EntityMetadataBase.get_metadata(None,
entity=MagicMock()) with an instance of that concrete subclass), so the test
uses a real object and will not break if the base implementation later calls
methods on self.
In
`@backend/tests/apps/owasp/management/commands/owasp_aggregate_entity_contributions_test.py`:
- Around line 460-462: Replace the weak mock assertion with a stronger one:
change the call to mock_aggregate.assert_called() in the test (the assertion
verifying aggregate_contributions was invoked) to
mock_aggregate.assert_called_once() so the test ensures aggregate_contributions
was invoked exactly once (consistent with the other test test_handle_with_offset
that uses assert_called_once()).
In
`@backend/tests/apps/owasp/management/commands/owasp_create_member_snapshot_test.py`:
- Around line 514-515: Rename the test method
test_generate_heatmap_data_empty_issues to clearly reflect that it tests an
issue outside the date range (e.g.,
test_generate_heatmap_data_issue_outside_date_range); update the method name and
any references to it so the test name matches the docstring "issue outside date
range" and adjust any test discovery references if present in tests or CI.
- Around line 962-975: In test_handle_with_slack_zero_messages, mock out
generate_channel_communications and ensure the mocked queryset iteration is
explicit: patch command.generate_channel_communications to return {} (like the
other generators), and set mock_msg_qs.__iter__.return_value = iter([]) on the
MagicMock (the same pattern used in test_handle_with_slack_messages) so the real
implementation isn't exercised when calling command.handle(username="testuser",
...); keep references to mock_message, mock_msg_qs,
command.generate_channel_communications, command.handle, and
mock_snapshot.messages.add to locate where to apply the changes.
In `@backend/tests/apps/owasp/management/commands/owasp_sync_posts_test.py`:
- Around line 218-239: Add an assertion that the command wrote a warning to
stderr in the no-YAML-match test: after calling command.handle() in
test_handle_no_yaml_match, assert that command.stderr.write.assert_called() to
mirror test_handle_missing_required_fields and confirm the warning branch was
exercised; keep existing assertions for mock_get_content.call_count,
mock_update_data.assert_not_called(), and
mock_bulk_save.assert_called_once_with([]).
In `@backend/tests/apps/owasp/management/commands/owasp_update_leaders_test.py`:
- Around line 248-249: The test currently patches User inside a call to
cmd.handle(model_name="invalid", threshold=75) but cmd.handle returns early due
to the model_name not in model_map guard, making the with
patch(f"{COMMAND_PATH}.User") unreachable; remove the dead with patch(...) block
(or move/replace it into a separate test that calls cmd.handle with a valid
model_name so User.objects.values(...) is exercised) and keep the assertion(s)
that verify the early-return behavior of cmd.handle(model_name="invalid")
instead.
In `@backend/tests/apps/slack/management/commands/slack_sync_messages_test.py`:
- Around line 951-962: Extract the duplicated SlackApiError construction (the
block creating SlackApiError, assigning error.response = MagicMock, setting
response.__getitem__, response.get, and response.headers) into a reusable pytest
fixture (e.g., rate_limit_error) at module scope (or in conftest.py) and update
the TestFetchRepliesRateLimit test methods (and other tests around the
duplicated blocks) to accept that fixture instead of rebuilding the mock; locate
occurrences by the SlackApiError instantiation and the MagicMock response setup
and replace them with the fixture injection.
In `@backend/tests/apps/slack/utils_test.py`:
- Line 223: The test function is misnamed with a trailing underscore: rename
test_get_gsoc_projects_ to test_get_gsoc_projects to match the naming
convention; update any references or test discovery expectations that call
test_get_gsoc_projects_ (e.g., in the file
backend/tests/apps/slack/utils_test.py) so they point to the new name and run
the tests to ensure no references remain.
- Around line 334-345: The test uses a mock queryset with __getitem__ but only
asserts result is not None; update test_get_sponsors_data to assert that
mock_queryset.__getitem__ was called with the expected slice (e.g. slice(0, 5)
or equivalent) and that the returned value equals the mocked slice value
(mock_sponsor list) from get_sponsors_data; locate the test and its mock setup
(mock_queryset and the patch of apps.owasp.models.sponsor.Sponsor.objects) and
add assertions for mock_queryset.__getitem__.assert_called_once_with(slice(0,
5)) and assert result == [mock_sponsor] (or the exact mocked return) to validate
slicing behavior of get_sponsors_data.
- Around line 252-276: The test reuses the variable name length for three
different meanings which masks bugs; update test_get_news_data to use distinct,
explicit variables (e.g., expected_total_items = 3, expected_limited_items = 2,
expected_get_calls = 2) and change assertions to use those names: assert
len(result) == expected_total_items, assert len(result_limited) ==
expected_limited_items, and assert mock_get.call_count == expected_get_calls;
keep references to mock_get, get_news_data, and OWASP_NEWS_URL unchanged.
backend/tests/apps/slack/management/commands/slack_sync_messages_test.py
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
backend/tests/apps/owasp/index/registry/committee_test.py (1)
9-19: Consider merging the twowithblocks into one.The
indexis constructed in a separate context from whereget_entities()is exercised. While functionally correct, the split is harder to follow at a glance and deviates from the nested/combined style used elsewhere in this PR (e.g.,chapter_test.py). A single compoundwithkeeps all test state within one scope.♻️ Proposed refactor
- with patch.object(CommitteeIndex, "__init__", lambda _: None): - index = CommitteeIndex() - - with patch("apps.owasp.index.registry.committee.Committee") as mock_committee: - mock_manager = mock_committee.active_committees - mock_manager.select_related.return_value = ["committee1", "committee2"] - - result = index.get_entities() - - mock_manager.select_related.assert_called_once_with("owasp_repository") - assert result == ["committee1", "committee2"] + with ( + patch.object(CommitteeIndex, "__init__", lambda _: None), + patch("apps.owasp.index.registry.committee.Committee") as mock_committee, + ): + index = CommitteeIndex() + mock_manager = mock_committee.active_committees + mock_manager.select_related.return_value = ["committee1", "committee2"] + + result = index.get_entities() + + mock_manager.select_related.assert_called_once_with("owasp_repository") + assert result == ["committee1", "committee2"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/index/registry/committee_test.py` around lines 9 - 19, Merge the two context managers into a single compound with to keep test setup and execution in one scope: patch CommitteeIndex.__init__ (lambda _: None) and patch "apps.owasp.index.registry.committee.Committee" together, then instantiate CommitteeIndex, set mock_committee.active_committees.select_related.return_value to ["committee1","committee2"], call index.get_entities(), and assert mock_manager.select_related was called with "owasp_repository" and the result equals the expected list; reference CommitteeIndex, CommitteeIndex.__init__, mock_committee (Committee), get_entities, and active_committees.select_related to locate the code to change.backend/tests/apps/slack/management/commands/slack_sync_messages_test.py (4)
752-900: Consider extracting repeated workspace/client mock setup into a shared fixture.Every test method in
TestSyncUserMessagesMissingBranches(and several other new classes) duplicates the same ~15-line block for mockingWorkspace,WebClient, and related queryset wiring. A@pytest.fixtureor a helper method on the class would eliminate this boilerplate and make each test focus on what it actually varies.♻️ Example fixture extraction
import pytest `@pytest.fixture`() def sync_command_env(mocker): """Common setup for _sync_user_messages tests.""" target = "apps.slack.management.commands.slack_sync_messages" mocker.patch(f"{target}.os.environ.get", return_value="xoxp-token") mocker.patch(f"{target}.time.sleep") mock_workspace = mocker.patch(f"{target}.Workspace") mock_ws = MagicMock(name="Test") mock_qs = MagicMock() mock_qs.exists.return_value = True mock_qs.__iter__.return_value = [mock_ws] mock_workspace.objects.all.return_value = mock_qs mock_webclient = mocker.patch(f"{target}.WebClient") mock_client = MagicMock() mock_webclient.return_value = mock_client command = Command() command.stdout = MagicMock() return command, mock_client, mocker.patch(f"{target}.Conversation"), mocker.patch(f"{target}.Message")Each test then only sets up its unique
search_messages.side_effectand assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/slack/management/commands/slack_sync_messages_test.py` around lines 752 - 900, Extract the repeated Workspace/WebClient/test command setup used across TestSyncUserMessagesMissingBranches into a shared pytest fixture (e.g., sync_command_env) or a class helper; move the block that patches target_module.os.environ.get, time.sleep, patches for Workspace (including mock_ws_instance/mock_qs wiring), WebClient (returning mock_client), and instantiation of Command/command.stdout into that fixture/helper, return the Command instance and mock_client and any patched Conversation/Message mocks so tests can just set mock_client.search_messages.side_effect and call command._sync_user_messages; update tests to accept/use the fixture (or call the helper) and remove the duplicated setup code from each test.
908-934: UnnecessaryMessagemock on line 910.
_fetch_messagesis patched to raise immediately, so theMessagemock is never accessed. You can safely remove themocker.patch(f"{self.target_module}.Message")line to reduce noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/slack/management/commands/slack_sync_messages_test.py` around lines 908 - 934, Remove the unnecessary mock of Message in the test: the line mocking f"{self.target_module}.Message" should be deleted because Command._fetch_messages is patched to raise a SlackApiError immediately and no code path uses Message during this test; update the test method test_fetch_conversation_api_error to rely only on the patched Command._fetch_messages and keep assertions that command.stdout.write was called after invoking Command._fetch_conversation with the mocked client and conversation.
794-831: Weak assertion —stdout.write.assert_called()doesn't verify the None-message skip path.
stdout.writeis invoked many times during_sync_user_messages(syncing header, workspace processing, etc.), soassert_called()will pass even if the_create_message returns Nonebranch is never reached. Consider asserting the specific skip message was written:💡 Suggested stronger assertion
command._sync_user_messages("U123", "2024-01-01", "2024-01-02", 0.1, 3) - command.stdout.write.assert_called() + skip_calls = [ + c for c in command.stdout.write.call_args_list + if "Skipped message" in str(c) + ] + assert len(skip_calls) == 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/slack/management/commands/slack_sync_messages_test.py` around lines 794 - 831, The test currently only checks stdout.write was called generally; update it to assert the specific skip log when _create_message returns None by checking command.stdout.write was called with a string containing the skip indicator and the message timestamp from the mocked search (e.g., ensure command.stdout.write.assert_any_call(...) is used with a value that includes "Skipping" and "1704110400"); locate this assertion around the Command() instantiation and _sync_user_messages call, referencing Command._sync_user_messages and Command._create_message to validate the None-path was exercised.
873-900: Same weak assertion astest_sync_message_create_returns_none.
command.stdout.write.assert_called()will always pass since the method writes to stdout unconditionally at the start. Consider asserting the specific error message substring (e.g.,"fatal_error") appears in one of thewritecalls to confirm the error handling branch was actually reached.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/slack/management/commands/slack_sync_messages_test.py` around lines 873 - 900, Replace the weak assertion that only checks that command.stdout.write was called with an assertion that the Slack error text was actually logged by the _sync_user_messages path: after invoking Command()._sync_user_messages, inspect command.stdout.write.call_args_list (or mock_calls) and assert that at least one call's first positional argument contains the substring "fatal_error" (e.g., assert any("fatal_error" in call.args[0] for call in command.stdout.write.call_args_list)), so the test verifies the error-handling branch in Command._sync_user_messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/apps/slack/apps_test.py`:
- Around line 39-44: Replace the fragile importlib.reload approach in
_restore_module() with a safe sys.modules snapshot/restore: before tests mutate
apps.slack.apps save original_module = sys.modules.get('apps.slack.apps'), and
in the finally restore by setting sys.modules['apps.slack.apps'] =
original_module if original_module is not None else del
sys.modules['apps.slack.apps'] (or pop) so the module is returned to its prior
state without re-executing module init; update any tests that call
_restore_module() in try/finally blocks to avoid swallowing the original
exception (do not catch exceptions in finally or re-raise the original
assertion) so failures propagate.
---
Duplicate comments:
In `@backend/tests/apps/slack/management/commands/slack_sync_messages_test.py`:
- Around line 757-792: The review comment contains a duplicate marker
([duplicate_comment]) even though the test test_sync_message_no_channel_id
already adds the assertion that Conversation.objects.get_or_create isn’t called;
remove the redundant duplicate tag from the review/comment so the approval is
clear and not repeated—locate the test test_sync_message_no_channel_id (which
exercises Command._sync_user_messages and mocks mock_client.search_messages) and
update the PR comment to keep only the [approve_code_changes] marker.
---
Nitpick comments:
In `@backend/tests/apps/owasp/index/registry/committee_test.py`:
- Around line 9-19: Merge the two context managers into a single compound with
to keep test setup and execution in one scope: patch CommitteeIndex.__init__
(lambda _: None) and patch "apps.owasp.index.registry.committee.Committee"
together, then instantiate CommitteeIndex, set
mock_committee.active_committees.select_related.return_value to
["committee1","committee2"], call index.get_entities(), and assert
mock_manager.select_related was called with "owasp_repository" and the result
equals the expected list; reference CommitteeIndex, CommitteeIndex.__init__,
mock_committee (Committee), get_entities, and active_committees.select_related
to locate the code to change.
In `@backend/tests/apps/slack/management/commands/slack_sync_messages_test.py`:
- Around line 752-900: Extract the repeated Workspace/WebClient/test command
setup used across TestSyncUserMessagesMissingBranches into a shared pytest
fixture (e.g., sync_command_env) or a class helper; move the block that patches
target_module.os.environ.get, time.sleep, patches for Workspace (including
mock_ws_instance/mock_qs wiring), WebClient (returning mock_client), and
instantiation of Command/command.stdout into that fixture/helper, return the
Command instance and mock_client and any patched Conversation/Message mocks so
tests can just set mock_client.search_messages.side_effect and call
command._sync_user_messages; update tests to accept/use the fixture (or call the
helper) and remove the duplicated setup code from each test.
- Around line 908-934: Remove the unnecessary mock of Message in the test: the
line mocking f"{self.target_module}.Message" should be deleted because
Command._fetch_messages is patched to raise a SlackApiError immediately and no
code path uses Message during this test; update the test method
test_fetch_conversation_api_error to rely only on the patched
Command._fetch_messages and keep assertions that command.stdout.write was called
after invoking Command._fetch_conversation with the mocked client and
conversation.
- Around line 794-831: The test currently only checks stdout.write was called
generally; update it to assert the specific skip log when _create_message
returns None by checking command.stdout.write was called with a string
containing the skip indicator and the message timestamp from the mocked search
(e.g., ensure command.stdout.write.assert_any_call(...) is used with a value
that includes "Skipping" and "1704110400"); locate this assertion around the
Command() instantiation and _sync_user_messages call, referencing
Command._sync_user_messages and Command._create_message to validate the
None-path was exercised.
- Around line 873-900: Replace the weak assertion that only checks that
command.stdout.write was called with an assertion that the Slack error text was
actually logged by the _sync_user_messages path: after invoking
Command()._sync_user_messages, inspect command.stdout.write.call_args_list (or
mock_calls) and assert that at least one call's first positional argument
contains the substring "fatal_error" (e.g., assert any("fatal_error" in
call.args[0] for call in command.stdout.write.call_args_list)), so the test
verifies the error-handling branch in Command._sync_user_messages.
Contributor
Author
|
@cubic-dev-ai review this pull request |
Contributor
@HarshitVerma109 I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/apps/slack/apps_test.py`:
- Around line 87-105: The test test_log_events_handles_exception accesses
handlers["use"] without first asserting the key exists; add a guard assertion
consistent with the other test by calling assert "use" in handlers immediately
after handlers, _ = self._reload_with_mock_app() (or after handlers are
assigned) so the test fails with a clear message if the handler capture changes;
update the test_log_events_handles_exception function to include this assert
before assigning log_events = handlers["use"].
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 33 files
Confidence score: 5/5
- This PR looks safe to merge; the only noted issue is a low-severity test maintainability concern rather than runtime behavior.
- Duplicated MagicMock queryset setup in
backend/tests/apps/owasp/management/commands/owasp_enrich_chapters_test.pycould make tests brittle if the mock needs updates in multiple places. - Pay close attention to
backend/tests/apps/owasp/management/commands/owasp_enrich_chapters_test.py- duplicated mock setup should be refactored into shared setup to ease maintenance.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/tests/apps/owasp/management/commands/owasp_enrich_chapters_test.py">
<violation number="1" location="backend/tests/apps/owasp/management/commands/owasp_enrich_chapters_test.py:118">
P3: Duplicated complex queryset mock setup in test_handle_geo_exception repeats the same MagicMock configuration already used in test_handle. This makes the tests brittle to changes and should be extracted into a shared fixture/helper.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
backend/tests/apps/owasp/management/commands/owasp_enrich_chapters_test.py
Outdated
Show resolved
Hide resolved
Contributor
Author
|
@cubic-dev-ai review this pull request |
Contributor
@HarshitVerma109 I have started the AI code review. It will take a few minutes to complete. |
arkid15r
requested changes
Feb 18, 2026
Collaborator
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Please update tests to be consistently grouped under classes (the inconsistency existed before your changes).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/tests/apps/owasp/management/commands/owasp_enrich_chapters_test.py (1)
104-133:test_handle_geo_exceptionis missing assertions for pre-exception steps and chapter stateThe test only asserts that
bulk_saveandgenerate_geo_locationwere called, but doesn't verify:
- That
generate_summaryandgenerate_suggested_locationwere actually invoked before the exception.- That
chapter.latitudeandchapter.longituderemainNoneafter the exception path (confirming no partial write).Consider adding:
mock_bulk_save.assert_called() mock_chapter.generate_geo_location.assert_called_once() + mock_chapter.generate_summary.assert_called_once() + mock_chapter.generate_suggested_location.assert_called_once() + assert mock_chapter.latitude is None + assert mock_chapter.longitude is NoneAlso, if production code catches a specific exception type (e.g.
requests.RequestException), using the broadExceptionhere won't catch a regression where that specific handling is removed. Consider using the same exception type that the productiontry/excepttargets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/management/commands/owasp_enrich_chapters_test.py` around lines 104 - 133, Update test_handle_geo_exception to assert pre-exception steps and post-exception state: assert mock_chapter.generate_summary.assert_called_once() and mock_chapter.generate_suggested_location.assert_called_once() to verify those were invoked, and assert that the chapter's latitude and longitude remain None (e.g., check mock_chapter.latitude is None and mock_chapter.longitude is None) to ensure no partial writes; also change the side_effect for generate_geo_location from raising a bare Exception to raising the same specific exception type the production code catches (e.g., requests.RequestException) so the test matches production error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/apps/owasp/management/commands/owasp_enrich_chapters_test.py`:
- Line 39: The test sets mock_active_chapters.__iter__.return_value to
iter(chapters_list) which produces a one-shot iterator that will be exhausted on
subsequent iterations; change the mock to provide a fresh iterator each time by
assigning mock_active_chapters.__iter__.side_effect = lambda:
iter(chapters_list) (or an equivalent callable) so every call to
iter(mock_active_chapters) yields a new iterator for use in loops inside the
command.
---
Nitpick comments:
In `@backend/tests/apps/owasp/management/commands/owasp_enrich_chapters_test.py`:
- Around line 104-133: Update test_handle_geo_exception to assert pre-exception
steps and post-exception state: assert
mock_chapter.generate_summary.assert_called_once() and
mock_chapter.generate_suggested_location.assert_called_once() to verify those
were invoked, and assert that the chapter's latitude and longitude remain None
(e.g., check mock_chapter.latitude is None and mock_chapter.longitude is None)
to ensure no partial writes; also change the side_effect for
generate_geo_location from raising a bare Exception to raising the same specific
exception type the production code catches (e.g., requests.RequestException) so
the test matches production error handling.
backend/tests/apps/owasp/management/commands/owasp_enrich_chapters_test.py
Outdated
Show resolved
Hide resolved
…arshitVerma109/Nest into feat/improve-test-coverage-100
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (18)
backend/tests/apps/api/decorators/cache_test.py (1)
13-30: LGTM — missing class docstring (nit).The refactor correctly encapsulates the previously module-level
test_generate_cache_keyintoTestGenerateCacheKey, aligning with the class-based style used byTestCacheResponse. All four parameterized cases are logically correct:
- The path-only case verifies no stray
?is appended for an empty query string.- The
?b=2&a=1case is a good explicit guard against any accidental query-parameter sorting ingenerate_key.One minor nit:
TestGenerateCacheKeyis missing a class-level docstring, whichTestCacheResponsehas at line 34.📝 Optional: add class docstring for consistency
class TestGenerateCacheKey: + """Test cases for the generate_key cache-key helper.""" + `@pytest.mark.parametrize`(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/api/decorators/cache_test.py` around lines 13 - 30, Add a class-level docstring to TestGenerateCacheKey to match the style of TestCacheResponse; open the TestGenerateCacheKey class definition and insert a concise docstring that summarizes the purpose (e.g., "Tests for generate_key cache key generation") immediately after the class line so that the class has the same documentation pattern as other test classes; ensure the docstring is a proper triple-quoted string placed before any methods like test_generate_cache_key and uses clear, short wording.backend/tests/apps/github/models/mixins/release_test.py (2)
61-61: Nit: prefer explicit== []overnot expectedfor the empty-author guard.
not expectedis falsy for[],None,0,False, etc. While only[]is reachable in this parametrize set, the intent is clearer with an explicit comparison.♻️ Proposed fix
- if attr == "idx_author" and not expected: + if attr == "idx_author" and expected == []:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/github/models/mixins/release_test.py` at line 61, Replace the falsy check for empty authors with an explicit empty-list comparison: in the test where the conditional reads "if attr == 'idx_author' and not expected", change it to check "expected == []" so the guard only triggers for an actual empty list; locate the conditional using the variables attr, expected and the literal 'idx_author' in the release_test.py test and update that expression accordingly.
52-53:idx_repositorytest doesn't exercise the.lower()call.The fixture path
"mock/repository"is already all-lowercase, soself.repository.path.lower()is a no-op and the lowercasing logic inidx_repositoryis never actually verified. Consider feeding a mixed-case path in a dedicated parametrize case.🔍 Proposed addition to the parametrize list
("idx_repository", "mock/repository"), + ("idx_repository", "mock/repository"), # lowercase transform tested belowAlternatively, update the fixture path and the expected value in the existing case:
- ("idx_repository", "mock/repository"), + ("idx_repository", "mock/repository"), # already covers exact matchA cleaner option is to add a dedicated entry that exercises
.lower():In the parametrize list add:
("idx_tag_name", "v1.0.0"), + ("idx_repository", "mock/repository"), # path already lowercase; no transformAnd in the fixture or a dedicated branch, supply an uppercase path:
elif attr == "idx_published_at" and expected is None: release_index_mixin_instance.published_at = None + elif attr == "idx_repository" and expected == "mock/repository" and ...: + release_index_mixin_instance.repository = MagicMock( + path="Mock/Repository", + project=MagicMock(nest_key="mock/project"), + )Or simply change the existing fixture's path to
"Mock/Repository"and update the expected value to"mock/repository"so.lower()is actually exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/github/models/mixins/release_test.py` around lines 52 - 53, The test currently uses the all-lowercase value "mock/repository" for the "idx_repository" case so the lowercasing in idx_repository (which calls self.repository.path.lower()) is not exercised; update the parametrize set in backend/tests/apps/github/models/mixins/release_test.py to include a case with a mixed-case repository path (e.g., "Mock/Repository") and set the expected "idx_repository" value to the lowercase form ("mock/repository"), or replace the existing fixture path with a mixed-case value and update the expected tuple accordingly so the .lower() behavior in idx_repository is actually verified.backend/tests/apps/owasp/index/search/chapter_test.py (1)
17-29: Use integer page values to match sibling tests.The
pageparametrize values are strings ("1","2") whileissue_test.pyandproject_test.pyuse plain integers. Theint(page)cast at the call site is an unnecessary indirection that adds noise without exercising any coercion path inget_chapters.♻️ Proposed fix
`@pytest.mark.parametrize`( ("query", "page", "expected_hits"), [ - ("security", "1", MOCKED_HITS), - ("web", "2", MOCKED_HITS), - ("", "1", MOCKED_HITS), + ("security", 1, MOCKED_HITS), + ("web", 2, MOCKED_HITS), + ("", 1, MOCKED_HITS), ], ) def test_get_chapters(self, query, page, expected_hits): with patch( "apps.owasp.index.search.chapter.raw_search", return_value=expected_hits ) as mock_raw_search: - result = get_chapters(query=query, page=int(page)) + result = get_chapters(query=query, page=page)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/index/search/chapter_test.py` around lines 17 - 29, The test uses string page values and then calls get_chapters with int(page); update the pytest.mark.parametrize tuple for ("query", "page", "expected_hits") to use integer page values (e.g., 1, 2) instead of "1", "2" and then call get_chapters(query=query, page=page) in test_get_chapters; this keeps the test consistent with sibling tests and removes the unnecessary int(...) coercion while still exercising get_chapters.backend/tests/apps/slack/commands/users_test.py (2)
75-92:test_users_handler_with_blocksomitschat_postMessagecall-count assertion.The test verifies the content of
chat_postMessage'sblocksargument but never asserts thatchat_postMessagewas actually called exactly once. If the handler skips posting (e.g., due to an exception or a falsy blocks return),call_argswould beNoneand the assertions on lines 89–92 would raiseTypeErrorrather than a clean failure, masking the real problem.♻️ Proposed fix
ack.assert_called_once() mock_client.conversations_open.assert_called_once_with(users="U123456") + assert mock_client.chat_postMessage.call_count == 1 blocks = mock_client.chat_postMessage.call_args[1]["blocks"] assert len(blocks) == len(mock_get_blocks.return_value)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/slack/commands/users_test.py` around lines 75 - 92, The test_users_handler_with_blocks is missing an explicit assertion that mock_client.chat_postMessage was called; update the test (test_users_handler_with_blocks) to assert mock_client.chat_postMessage.assert_called_once() (or assert_called_once_with(...) if you prefer) immediately after invoking Users().handler(ack=ack, command=mock_command, client=mock_client) and before accessing mock_client.chat_postMessage.call_args, so the test fails clearly if the handler did not post a message.
52-54: Remove duplicateack.assert_called_once()assertion.Line 54 is an exact duplicate of line 52; the second call is redundant.
🧹 Proposed fix
ack.assert_called_once() - ack.assert_called_once() assert mock_client.chat_postMessage.call_count == expected_calls🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/slack/commands/users_test.py` around lines 52 - 54, Remove the duplicate assertion call by deleting the second ack.assert_called_once() so the test only asserts ack.assert_called_once() once; locate the duplicate in backend/tests/apps/slack/commands/users_test.py (the repeated ack.assert_called_once() lines) and keep a single assertion to avoid redundancy.backend/tests/apps/api/rest/v0/structured_search_test.py (2)
91-101: Missing positive assertion for the exact lookup suffix.The test only checks that
name__icontainsis absent (line 101), but it never verifies thatname__exactis actually present. If the filter were applied with a different (or empty) lookup, the test would still pass, leaving the intended behavior unvalidated.♻️ Suggested improvement
qs.filter.assert_called_once() args, _ = qs.filter.call_args assert "name__icontains" not in str(args[0]) + assert "name__exact" in str(args[0])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/api/rest/v0/structured_search_test.py` around lines 91 - 101, The test test_string_field_with_exact_lookup currently only asserts that "name__icontains" is absent; update it to also assert that the filter call includes the exact lookup by checking that "name__exact" appears in the filter args after apply_structured_search; locate the variables schema_with_exact, qs (make_queryset()), the call apply_structured_search(qs, "name:test", schema_with_exact) and the mocked qs.filter call, and add an assertion that "name__exact" is present in str(args[0]) (or equivalent) to confirm the exact lookup was applied.
140-151: Weak assertion — doesn't verify the unknown field was excluded from the Q object.
filter.assert_called_once()only confirms the function reached the filter call without erroring. It does not verify that the unrecognised field"query"was actually skipped (i.e., that the resulting Q object is empty). A poorly behaved change could include the unknown field in the filter and this test would still pass.♻️ Suggested improvement
apply_structured_search(qs, "test", FIELD_SCHEMA) qs.filter.assert_called_once() + args, _ = qs.filter.call_args + assert "query" not in str(args[0])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/api/rest/v0/structured_search_test.py` around lines 140 - 151, Update the test_condition_field_not_in_schema_is_skipped test to assert the actual Q object passed to qs.filter is empty rather than just that filter was called: after calling apply_structured_search(qs, "test", FIELD_SCHEMA) fetch qs.filter.call_args (or call_args_list[0]) and assert the first positional argument is an empty django.db.models.Q() (or that its .children is empty), using Q from django.db.models so you verify apply_structured_search (and QueryParser mock) skipped the unknown "query" field.backend/tests/apps/github/management/commands/github_update_owasp_organization_test.py (1)
103-114:PaginatedListMockis duplicated across two test methods.The identical helper class appears in both
test_handle(lines 103–114) andtest_handle_full_sync_with_errors_and_repo_linking(lines 195–206). Extract it to the class or module level to reduce duplication.Also applies to: 195-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/github/management/commands/github_update_owasp_organization_test.py` around lines 103 - 114, The PaginatedListMock helper class is duplicated in test_handle and test_handle_full_sync_with_errors_and_repo_linking; extract PaginatedListMock to the module/class scope so both tests reuse the same definition. Move the class definition currently inside test_handle (and remove the duplicate inside test_handle_full_sync_with_errors_and_repo_linking), placing a single PaginatedListMock definition at the top of the test module or test class so both tests reference that symbol.backend/tests/apps/github/management/commands/github_add_related_repositories_test.py (2)
67-77: Repeatedactive_projectsmock setup across tests.The pattern of configuring
__iter__,count,__getitem__, andorder_byon the active_projects mock is repeated nearly identically in every test method. Consider extracting a helper (e.g.,_setup_active_projects(self, projects_list)) on the test class to reduce boilerplate, similar to the_setup_organizations_mockpattern ingithub_update_related_organizations_test.py.Also applies to: 118-123, 171-176, 216-221, 270-274, 316-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/github/management/commands/github_add_related_repositories_test.py` around lines 67 - 77, The repeated setup of mock_active_projects (setting __iter__, count, __getitem__, and order_by) should be extracted into a reusable helper on the test class (e.g., _setup_active_projects(self, projects_list)) to remove boilerplate; implement _setup_active_projects to create/configure and return a MagicMock with __iter__.return_value = iter(projects_list), count.return_value = len(projects_list), a __getitem__ lambda handling int vs slice, and order_by.return_value = the mock itself, then replace each in-test block that constructs mock_active_projects with calls to this helper (update tests at the multiple locations noted).
117-149: Missingcommand.stdoutmock in several test methods.
test_handle(line 83) setscommand.stdout = mock.MagicMock()to suppress output, buttest_handle_unknown_object_exception,test_handle_no_repository_path,test_handle_unknown_object_exception_non_404,test_handle_sync_repository_exception, andtest_handle_sync_repository_returns_none_organizationdo not. If the command writes tostdoutin these paths, the tests will emit output to the console during test runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/github/management/commands/github_add_related_repositories_test.py` around lines 117 - 149, Add a mock for command.stdout in the specified test methods so test output is suppressed: in test_handle_unknown_object_exception, test_handle_no_repository_path, test_handle_unknown_object_exception_non_404, test_handle_sync_repository_exception, and test_handle_sync_repository_returns_none_organization set command.stdout = mock.MagicMock() (same as test_handle does) before invoking command.handle; this ensures any writes during command.handle (and related calls like Project.bulk_save, mock_gh_client.get_repo, mock_sync_repository) do not print to the console during test runs.backend/tests/apps/github/management/commands/github_enrich_issues_test.py (1)
177-209: Misleading test name:test_handle_with_chunked_savedoesn't test chunking.The test creates 50 issues and asserts
bulk_saveis called exactly once with all 50 items (lines 205–209). This validates single-batch processing, not chunked saving. The docstring ("correctly processes multiple issues") is more accurate than the method name.Consider renaming to
test_handle_processes_all_issuesor similar.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/github/management/commands/github_enrich_issues_test.py` around lines 177 - 209, The test method name test_handle_with_chunked_save is misleading because it does not verify chunked saves; rename the test to something like test_handle_processes_all_issues (and update the docstring to match) so the name reflects that it asserts a single bulk_save call with all 50 issues; locate the test function definition in github_enrich_issues_test.py, change the method name and any internal references (e.g., any test runner/tests mapping) and keep the existing assertions around mock_issue_class.bulk_save and kwargs["fields"] unchanged.backend/tests/apps/api/rest/v0/chapter_test.py (1)
37-54:MockMember/MockEntityMemberduplicated inproject_test.py.These two helper classes (lines 37-44) are identical to the ones defined inside
test_project_serializer_validationinproject_test.py(lines 33-40). If either evolves (e.g., the resolver adds a field), both copies need updating. Consider extracting them to a sharedconftest.pyfixture or a module-level definition.♻️ Suggested extraction to conftest.py
# In backend/tests/apps/api/rest/v0/conftest.py (new or existing) import pytest class MockMember: def __init__(self, login): self.login = login class MockEntityMember: def __init__(self, name, login=None): self.member = MockMember(login) if login else None self.member_name = name `@pytest.fixture` def make_entity_member(): return MockEntityMember🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/api/rest/v0/chapter_test.py` around lines 37 - 54, The MockMember and MockEntityMember classes are duplicated between chapter_test.py and project_test.py; extract them to a shared test location (e.g., backend/tests/apps/api/rest/v0/conftest.py or a module-level helpers file) and import or reference the shared definitions from both test files so updates are centralized; specifically move the MockMember and MockEntityMember implementations out of test functions into the shared conftest/module and change chapter_test.py and project_test.py to use the shared MockMember/MockEntityMember (or a fixture like make_entity_member) instead of their local copies.backend/tests/apps/api/rest/v0/common_test.py (1)
7-30: Consider adding a test case forNonedefaults.Both parametrize cases supply all four fields with float values.
LocationFilterdeclares all fields asfloat | None = Field(None, ...), so the all-Nonedefault path is untested. Adding a third case (empty dict{}) would cover that branch.♻️ Suggested additional test case
{ "latitude_gte": 15.0, "latitude_lte": 25.0, "longitude_gte": 20.0, "longitude_lte": 30.0, }, + { + "latitude_gte": None, + "latitude_lte": None, + "longitude_gte": None, + "longitude_lte": None, + }, ], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/api/rest/v0/common_test.py` around lines 7 - 30, Add a third parametrize case to test the None/default path: update the test_location_filter_validation parameter list to include an empty dict {} as a third location_data case, then assert that LocationFilter(**location_data) yields latitude_gte, latitude_lte, longitude_gte, and longitude_lte all equal to None for that case; reference the existing test function test_location_filter_validation and the LocationFilter model to locate where to add this case.backend/tests/apps/api/rest/v0/committee_test.py (1)
10-10: Missing class-level docstring — inconsistent with existing test classes.
TestListCommittees(line 47) andTestGetCommittee(line 75) both have docstrings, as do their counterparts in the other test files. The new classTestCommitteeSerializerValidation(and its equivalents inchapter_test.py,common_test.py,project_test.py,event_test.py) lack docstrings. Consider adding one for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/api/rest/v0/committee_test.py` at line 10, Add a class-level docstring to the TestCommitteeSerializerValidation test class to match the style used by TestListCommittees and TestGetCommittee; locate the class named TestCommitteeSerializerValidation and add a concise docstring describing the purpose (e.g., "Tests for validating Committee serializer input and error responses"), and apply the same change to the equivalent classes in chapter_test.py, common_test.py, project_test.py, and event_test.py to keep test-file docstring consistency.backend/tests/apps/owasp/management/commands/owasp_enrich_chapters_test.py (1)
104-133: Consider asserting thatlatitude/longituderemainNoneafter a geo exception.The test correctly verifies that
bulk_saveis still invoked and thatgenerate_geo_locationwas called. An additional assertion that the chapter's geo fields were not mutated would make the exception-path contract explicit and guard against future regressions where a partial update slips through.✨ Suggested addition
mock_bulk_save.assert_called() mock_chapter.generate_geo_location.assert_called_once() + assert mock_chapter.latitude is None + assert mock_chapter.longitude is None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/management/commands/owasp_enrich_chapters_test.py` around lines 104 - 133, In test_handle_geo_exception, add assertions after command.handle to verify that the chapter's geo fields were not mutated when generate_geo_location raised an exception: assert mock_chapter.latitude is None and assert mock_chapter.longitude is None (or the model's equivalent field names) so the test ensures Chapter instances retain None latitude/longitude after the geo exception; keep these assertions using the existing mock_chapter used in the test and place them just before the final mock_bulk_save and generate_geo_location assertions or immediately after command.handle.backend/tests/apps/slack/utils_test.py (2)
336-343: Missing test foryaml.scanner.ScannerErrorerror path.
get_staff_datacatches bothRequestExceptionandyaml.scanner.ScannerError, but only the former is exercised. If the goal of this PR is 100% branch coverage, the YAML parsing error branch is not covered. Consider adding:➕ Suggested additional test
def test_get_staff_data_yaml_error(self, monkeypatch): """Test get_staff_data handles YAML parsing errors gracefully.""" import yaml mock_response = Mock() mock_response.text = ": invalid: yaml: [" mock_get = Mock(return_value=mock_response) monkeypatch.setattr("requests.get", mock_get) get_staff_data.cache_clear() result = get_staff_data() assert result is None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/slack/utils_test.py` around lines 336 - 343, Add a new unit test covering the YAML parsing error path for get_staff_data: mock requests.get to return a response-like object whose .text is invalid YAML (so yaml.safe_load raises yaml.scanner.ScannerError), call get_staff_data.cache_clear(), invoke get_staff_data() and assert it returns None; reference the existing test_get_staff_data_request_exception pattern and use get_staff_data and requests.get when locating where to add the new test.
275-287:lengthis reused for three different semantics — clarify the HTTP call count assertion.
lengthis first set to3(news item count), then reassigned to2(limit), and then silently reused for the HTTP call-count assertion at line 287. Because the limit and expected call count happen to share the value2, the assertion appears to check "2 results" rather than "exactly 2 HTTP requests", which is the actual intent.♻️ Proposed refactor
- length = 2 result_limited = get_news_data(limit=2) - assert len(result_limited) == length + assert len(result_limited) == 2 mock_get.assert_called_with(OWASP_NEWS_URL, timeout=30) result2 = get_news_data() - assert mock_get.call_count == length + assert mock_get.call_count == 2 # default (limit=10) + limited (limit=2); third call is a cache hit assert result == result2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/slack/utils_test.py` around lines 275 - 287, The test reuses the variable length for different meanings (3 items, 2 limit) and then uses it to assert mock_get.call_count, making the intent unclear; update the test around get_news_data, replacing reused length with clearly named variables (e.g., expected_total_items, expected_limited_items, expected_http_calls) and assert mock_get.call_count == expected_http_calls; reference get_news_data, mock_get, and OWASP_NEWS_URL to locate and update the assertions so the HTTP call-count check explicitly compares to the intended number of requests rather than a reused length variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/apps/api/rest/v0/structured_search_test.py`:
- Around line 42-49: The test named
test_invalid_syntax_returns_original_queryset is misleading because
apply_structured_search(qs, "stars:!!!", FIELD_SCHEMA) actually calls
qs.filter(Q()) after skipping the invalid numeric value; update the test to
reflect that behavior: rename the test to something like
test_invalid_number_skipped_applies_empty_filter, and change/assertions to
verify that qs.filter.assert_called_once() and that the returned value equals
qs.filter.return_value (i.e., capture result = apply_structured_search(...) and
assert result is qs.filter.return_value), while keeping the existing check that
"stars_count" is not in the filter args; refer to the test function name,
apply_structured_search, make_queryset, FIELD_SCHEMA, and qs.filter when making
these edits.
In
`@backend/tests/apps/github/management/commands/github_update_owasp_organization_test.py`:
- Around line 63-64: expected_calls for the full-sync (repository=None) cases
are never asserted; update the assertion block that currently only checks
expected_calls when repository_name is truthy to also verify the None branch:
iterate expected_calls entries where the first element is None and assert the
mocked update_data calls match those entries (either by checking
command.update_data.call_args_list contains calls with the expected (None, id,
counts) tuples or by asserting command.update_data.call_count/filtered calls
equal the expected counts), instead of only asserting
command.stdout.write.call_count > 0; use the existing symbols expected_calls,
repository_name, and command.update_data / command.stdout.write to locate and
implement the checks.
In
`@backend/tests/apps/github/management/commands/github_update_related_organizations_test.py`:
- Around line 80-85: Remove the dead pytest fixture named mock_get_github_client
from the test module; it is unused by TestGithubUpdateRelatedOrganizations and
duplicates the existing patch applied in the autouse setup fixture (which uses
monkeypatch.setattr to patch get_github_client). Delete the fixture definition
block (the function mock_get_github_client and its mock.patch context) so only
the autouse setup that patches get_github_client remains.
In `@backend/tests/apps/slack/commands/users_test.py`:
- Line 40: The tests mutate the global Django settings by assigning
settings.SLACK_COMMANDS_ENABLED directly, which leaks state; update the tests
(e.g., test_users_handler and test_users_handler_with_blocks) to accept the
pytest-django settings fixture and set settings.SLACK_COMMANDS_ENABLED =
commands_enabled using that fixture (or use
settings.setattr('SLACK_COMMANDS_ENABLED', commands_enabled)) so the change is
automatically restored after each test; remove any direct assignments to the
global settings object and ensure the fixture name is added to the test
signature where you adjust SLACK_COMMANDS_ENABLED.
In `@backend/tests/apps/slack/utils_test.py`:
- Around line 233-260: The test for get_gsoc_projects is missing a cache clear
which can cause stale results; before calling get_gsoc_projects("2023") (and
before the second call), clear the function cache by invoking
get_gsoc_projects.cache_clear() so the mocked get_projects (mock_get_projects)
is actually exercised and mock_get_projects.call_count remains deterministic;
modify the TestGetGsocProjects.test_get_gsoc_projects setup to call
get_gsoc_projects.cache_clear() prior to the first invocation.
---
Nitpick comments:
In `@backend/tests/apps/api/decorators/cache_test.py`:
- Around line 13-30: Add a class-level docstring to TestGenerateCacheKey to
match the style of TestCacheResponse; open the TestGenerateCacheKey class
definition and insert a concise docstring that summarizes the purpose (e.g.,
"Tests for generate_key cache key generation") immediately after the class line
so that the class has the same documentation pattern as other test classes;
ensure the docstring is a proper triple-quoted string placed before any methods
like test_generate_cache_key and uses clear, short wording.
In `@backend/tests/apps/api/rest/v0/chapter_test.py`:
- Around line 37-54: The MockMember and MockEntityMember classes are duplicated
between chapter_test.py and project_test.py; extract them to a shared test
location (e.g., backend/tests/apps/api/rest/v0/conftest.py or a module-level
helpers file) and import or reference the shared definitions from both test
files so updates are centralized; specifically move the MockMember and
MockEntityMember implementations out of test functions into the shared
conftest/module and change chapter_test.py and project_test.py to use the shared
MockMember/MockEntityMember (or a fixture like make_entity_member) instead of
their local copies.
In `@backend/tests/apps/api/rest/v0/committee_test.py`:
- Line 10: Add a class-level docstring to the TestCommitteeSerializerValidation
test class to match the style used by TestListCommittees and TestGetCommittee;
locate the class named TestCommitteeSerializerValidation and add a concise
docstring describing the purpose (e.g., "Tests for validating Committee
serializer input and error responses"), and apply the same change to the
equivalent classes in chapter_test.py, common_test.py, project_test.py, and
event_test.py to keep test-file docstring consistency.
In `@backend/tests/apps/api/rest/v0/common_test.py`:
- Around line 7-30: Add a third parametrize case to test the None/default path:
update the test_location_filter_validation parameter list to include an empty
dict {} as a third location_data case, then assert that
LocationFilter(**location_data) yields latitude_gte, latitude_lte,
longitude_gte, and longitude_lte all equal to None for that case; reference the
existing test function test_location_filter_validation and the LocationFilter
model to locate where to add this case.
In `@backend/tests/apps/api/rest/v0/structured_search_test.py`:
- Around line 91-101: The test test_string_field_with_exact_lookup currently
only asserts that "name__icontains" is absent; update it to also assert that the
filter call includes the exact lookup by checking that "name__exact" appears in
the filter args after apply_structured_search; locate the variables
schema_with_exact, qs (make_queryset()), the call apply_structured_search(qs,
"name:test", schema_with_exact) and the mocked qs.filter call, and add an
assertion that "name__exact" is present in str(args[0]) (or equivalent) to
confirm the exact lookup was applied.
- Around line 140-151: Update the test_condition_field_not_in_schema_is_skipped
test to assert the actual Q object passed to qs.filter is empty rather than just
that filter was called: after calling apply_structured_search(qs, "test",
FIELD_SCHEMA) fetch qs.filter.call_args (or call_args_list[0]) and assert the
first positional argument is an empty django.db.models.Q() (or that its
.children is empty), using Q from django.db.models so you verify
apply_structured_search (and QueryParser mock) skipped the unknown "query"
field.
In
`@backend/tests/apps/github/management/commands/github_add_related_repositories_test.py`:
- Around line 67-77: The repeated setup of mock_active_projects (setting
__iter__, count, __getitem__, and order_by) should be extracted into a reusable
helper on the test class (e.g., _setup_active_projects(self, projects_list)) to
remove boilerplate; implement _setup_active_projects to create/configure and
return a MagicMock with __iter__.return_value = iter(projects_list),
count.return_value = len(projects_list), a __getitem__ lambda handling int vs
slice, and order_by.return_value = the mock itself, then replace each in-test
block that constructs mock_active_projects with calls to this helper (update
tests at the multiple locations noted).
- Around line 117-149: Add a mock for command.stdout in the specified test
methods so test output is suppressed: in test_handle_unknown_object_exception,
test_handle_no_repository_path, test_handle_unknown_object_exception_non_404,
test_handle_sync_repository_exception, and
test_handle_sync_repository_returns_none_organization set command.stdout =
mock.MagicMock() (same as test_handle does) before invoking command.handle; this
ensures any writes during command.handle (and related calls like
Project.bulk_save, mock_gh_client.get_repo, mock_sync_repository) do not print
to the console during test runs.
In `@backend/tests/apps/github/management/commands/github_enrich_issues_test.py`:
- Around line 177-209: The test method name test_handle_with_chunked_save is
misleading because it does not verify chunked saves; rename the test to
something like test_handle_processes_all_issues (and update the docstring to
match) so the name reflects that it asserts a single bulk_save call with all 50
issues; locate the test function definition in github_enrich_issues_test.py,
change the method name and any internal references (e.g., any test runner/tests
mapping) and keep the existing assertions around mock_issue_class.bulk_save and
kwargs["fields"] unchanged.
In
`@backend/tests/apps/github/management/commands/github_update_owasp_organization_test.py`:
- Around line 103-114: The PaginatedListMock helper class is duplicated in
test_handle and test_handle_full_sync_with_errors_and_repo_linking; extract
PaginatedListMock to the module/class scope so both tests reuse the same
definition. Move the class definition currently inside test_handle (and remove
the duplicate inside test_handle_full_sync_with_errors_and_repo_linking),
placing a single PaginatedListMock definition at the top of the test module or
test class so both tests reference that symbol.
In `@backend/tests/apps/github/models/mixins/release_test.py`:
- Line 61: Replace the falsy check for empty authors with an explicit empty-list
comparison: in the test where the conditional reads "if attr == 'idx_author' and
not expected", change it to check "expected == []" so the guard only triggers
for an actual empty list; locate the conditional using the variables attr,
expected and the literal 'idx_author' in the release_test.py test and update
that expression accordingly.
- Around line 52-53: The test currently uses the all-lowercase value
"mock/repository" for the "idx_repository" case so the lowercasing in
idx_repository (which calls self.repository.path.lower()) is not exercised;
update the parametrize set in
backend/tests/apps/github/models/mixins/release_test.py to include a case with a
mixed-case repository path (e.g., "Mock/Repository") and set the expected
"idx_repository" value to the lowercase form ("mock/repository"), or replace the
existing fixture path with a mixed-case value and update the expected tuple
accordingly so the .lower() behavior in idx_repository is actually verified.
In `@backend/tests/apps/owasp/index/search/chapter_test.py`:
- Around line 17-29: The test uses string page values and then calls
get_chapters with int(page); update the pytest.mark.parametrize tuple for
("query", "page", "expected_hits") to use integer page values (e.g., 1, 2)
instead of "1", "2" and then call get_chapters(query=query, page=page) in
test_get_chapters; this keeps the test consistent with sibling tests and removes
the unnecessary int(...) coercion while still exercising get_chapters.
In `@backend/tests/apps/owasp/management/commands/owasp_enrich_chapters_test.py`:
- Around line 104-133: In test_handle_geo_exception, add assertions after
command.handle to verify that the chapter's geo fields were not mutated when
generate_geo_location raised an exception: assert mock_chapter.latitude is None
and assert mock_chapter.longitude is None (or the model's equivalent field
names) so the test ensures Chapter instances retain None latitude/longitude
after the geo exception; keep these assertions using the existing mock_chapter
used in the test and place them just before the final mock_bulk_save and
generate_geo_location assertions or immediately after command.handle.
In `@backend/tests/apps/slack/commands/users_test.py`:
- Around line 75-92: The test_users_handler_with_blocks is missing an explicit
assertion that mock_client.chat_postMessage was called; update the test
(test_users_handler_with_blocks) to assert
mock_client.chat_postMessage.assert_called_once() (or
assert_called_once_with(...) if you prefer) immediately after invoking
Users().handler(ack=ack, command=mock_command, client=mock_client) and before
accessing mock_client.chat_postMessage.call_args, so the test fails clearly if
the handler did not post a message.
- Around line 52-54: Remove the duplicate assertion call by deleting the second
ack.assert_called_once() so the test only asserts ack.assert_called_once() once;
locate the duplicate in backend/tests/apps/slack/commands/users_test.py (the
repeated ack.assert_called_once() lines) and keep a single assertion to avoid
redundancy.
In `@backend/tests/apps/slack/utils_test.py`:
- Around line 336-343: Add a new unit test covering the YAML parsing error path
for get_staff_data: mock requests.get to return a response-like object whose
.text is invalid YAML (so yaml.safe_load raises yaml.scanner.ScannerError), call
get_staff_data.cache_clear(), invoke get_staff_data() and assert it returns
None; reference the existing test_get_staff_data_request_exception pattern and
use get_staff_data and requests.get when locating where to add the new test.
- Around line 275-287: The test reuses the variable length for different
meanings (3 items, 2 limit) and then uses it to assert mock_get.call_count,
making the intent unclear; update the test around get_news_data, replacing
reused length with clearly named variables (e.g., expected_total_items,
expected_limited_items, expected_http_calls) and assert mock_get.call_count ==
expected_http_calls; reference get_news_data, mock_get, and OWASP_NEWS_URL to
locate and update the assertions so the HTTP call-count check explicitly
compares to the intended number of requests rather than a reused length
variable.
backend/tests/apps/github/management/commands/github_update_owasp_organization_test.py
Show resolved
Hide resolved
backend/tests/apps/github/management/commands/github_update_related_organizations_test.py
Outdated
Show resolved
Hide resolved
Contributor
Author
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3991 +/- ##
==========================================
+ Coverage 99.15% 99.79% +0.64%
==========================================
Files 517 517
Lines 15914 15914
Branches 2170 2168 -2
==========================================
+ Hits 15779 15881 +102
+ Misses 66 3 -63
+ Partials 69 30 -39
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 29 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
arkid15r
approved these changes
Feb 18, 2026
Collaborator
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
LGTM
Thanks for addressing this.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
Resolves #3978
Improved backend test coverage to 100%
Checklist
make check-testlocally: all warnings addressed, tests passed