Skip to content

feat(discord_activity_tracker): docstrings, tests, and discord.py failure classification#201

Merged
wpak-ai merged 3 commits into
cppalliance:developfrom
leostar0412:feat/discord-tracker-docs-tests
May 14, 2026
Merged

feat(discord_activity_tracker): docstrings, tests, and discord.py failure classification#201
wpak-ai merged 3 commits into
cppalliance:developfrom
leostar0412:feat/discord-tracker-docs-tests

Conversation

@leostar0412
Copy link
Copy Markdown
Collaborator

@leostar0412 leostar0412 commented May 13, 2026

Summary

  • Expand Discord Activity Tracker package and module documentation: app __init__, management commands, sync package, models, and services (module + function docstrings aligned with the service API).
  • Classify discord.py HTTP and auth-style failures in core.errors.classify_failure (e.g. HTTPException status codes, login / privileged intents / client errors) alongside existing HTTP and transport handling.
  • Add broad unit/integration-style test coverage for backfill and run commands, chat exporter branches, export sync, exporter windows, failure classification, Pinecone runner, preprocessor, staging schema, task sync/markdown, workspace staging, and related paths; extend existing sync tests where needed.
  • Refresh docs/Service_API.md and docs/service_api/discord_activity_tracker.md to match the documented behavior and commands.

Test plan

  • pytest (or project test command) for discord_activity_tracker and core tests touched by this change
  • Smoke: manage.py run_discord_activity_tracker / backfill_discord_activity_tracker help or dry-run if applicable in your environment

Closes #190, closes #192

Summary by CodeRabbit

  • Documentation

    • Expanded and clarified app, service API, management command, and sync/export docs, including service contracts and bulk upsert semantics.
  • Bug Fixes / Improvements

    • Improved classification of Discord HTTP errors and specific Discord exception cases for more appropriate failure categories.
  • Tests

    • Added extensive unit and integration tests covering backfill, sync, export/markdown, preprocessing, Pinecone sync, staging, workspace tooling, and command workflows.

Review Change Stack

@leostar0412 leostar0412 self-assigned this May 13, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 13, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: cd94644e-1499-4dca-bf89-60114ab70464

📥 Commits

Reviewing files that changed from the base of the PR and between ed3135f and 2b3407e.

📒 Files selected for processing (1)
  • discord_activity_tracker/tests/test_sync_chat_exporter.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • discord_activity_tracker/tests/test_sync_chat_exporter.py

📝 Walkthrough

Walkthrough

Adds discord.py-aware failure classification to core/errors, expands docstrings across discord_activity_tracker (models, services, commands, packages), and introduces extensive tests covering backfill, sync, export, preprocessing, Pinecone sync, and management-command behaviors.

Changes

Discord Tracker: Docs & Tests

Layer / File(s) Summary
Discord HTTP exception classification
core/errors.py
Extended classify_failure to classify discord.py exceptions by status and map specific discord exception names to AUTH.
Package and module docstrings
discord_activity_tracker/__init__.py, discord_activity_tracker/management/__init__.py, discord_activity_tracker/management/commands/__init__.py, discord_activity_tracker/sync/__init__.py
Added module-level docstrings describing app scope, command package, and sync helpers.
Management command documentation
discord_activity_tracker/management/commands/backfill_discord_activity_tracker.py, discord_activity_tracker/management/commands/run_discord_activity_tracker.py
Expanded module/class/Command docstrings documenting input layout, phases, dry-run/delete semantics, Pinecone sync option, and usage examples.
Model class documentation
discord_activity_tracker/models.py
Expanded docstrings for DiscordServer, DiscordChannel, DiscordMessage, and DiscordReaction.
Service layer documentation
discord_activity_tracker/services.py
Expanded docstrings detailing contracts (single-writer policy, DB-only guarantees), bulk upsert semantics, transaction orchestration, and return-count rule for bulk_process_message_batch.
Service API documentation
docs/Service_API.md, docs/service_api/discord_activity_tracker.md
Added service contract details and clarified that bulk_process_message_batch returns len(message_data_list) when non-empty.
Backfill command tests
discord_activity_tracker/tests/test_backfill_command_extra.py
Tests for json display path, pinecone sync dry-run/skip, get_collector skip_pinecone normalization, malformed JSON handling, and async persist.
Chat exporter branch coverage
discord_activity_tracker/tests/test_chat_exporter_branch_coverage.py
Branch tests for binary detection, subprocess error handling, export_guild_to_json error branches, and stdout parsing.
Markdown export & git-sync tests
discord_activity_tracker/tests/test_export_sync_coverage.py
Extensive markdown content and git-sync tests covering timestamps, replies, code fences, attachments, per-day exports, commit/push flows, and export_and_push behaviors.
Exporter window timestamp tests
discord_activity_tracker/tests/test_exporter_window.py
Tests for latest message timestamp retrieval, deleted-message filtering, and channel allowlist handling.
Failure classification tests
discord_activity_tracker/tests/test_failure_classification.py
Unit tests synthesizing discord.errors exceptions and asserting mappings to CollectorFailureCategory.
Pinecone runner tests
discord_activity_tracker/tests/test_pinecone_runner_coverage.py
Tests for dry-run skip, configuration-based skipping, successful invocation, and swallowed errors.
Preprocessor & document generation tests
discord_activity_tracker/tests/test_preprocessor_extra.py
Tests for reply chains, orphan messages, chain→document behavior, duplicate doc_id de-duplication, and nothing-to-sync logging.
Run command orchestration tests
discord_activity_tracker/tests/test_run_command_coverage.py
Comprehensive tests for date-bound resolution, task routing, skip flags, error propagation, and async persistence.
Staging schema & validation tests
discord_activity_tracker/tests/test_staging_schema_extra.py
Tests for staging schema bundle contents, write_staging_json_schema output, and normalized message validation for blank edited timestamps.
Sync exporter and client tests
discord_activity_tracker/tests/test_sync_chat_exporter.py, discord_activity_tracker/tests/test_sync_client.py
Tests for embed-key ignoring, rate-limit handling, message-type labeling, message dict conversion, and shutdown error logging.
Discord sync task end-to-end tests
discord_activity_tracker/tests/test_task_discord_sync_coverage.py
End-to-end tests covering staging validation, raw promotion, allowlist filtering, error handling, and cleanup behaviors.
Markdown export task tests
discord_activity_tracker/tests/test_task_markdown_coverage.py
Tests for skip paths, missing context path, dry-run output, server-not-in-db reporting, success/warn flows, and auto-commit path.
Workspace cleanup & schema script tests
discord_activity_tracker/tests/test_workspace_clear_staging.py, discord_activity_tracker/tests/test_write_staging_json_schema_script.py
Tests for staging dir cleanup and script main() printed path.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

enhancement, documentation

Suggested reviewers

  • jonathanMLDev
  • wpak-ai

"I'm a rabbit in the patch of code so neat,
Docstrings sown, and tests all in a neat fleet.
Rate limits caught, exports tidied and true,
Pinecone and backfill now have tests too.
Hoppity hop — the CI will greet!"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.28% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly summarizes the main changes: adding docstrings, tests, and discord.py failure classification to the discord_activity_tracker module.
Linked Issues check ✅ Passed The PR comprehensively addresses both linked issues: #190 (test coverage with 15+ tests, 80%+ line coverage, failure classification) and #192 (docstrings for services, models, commands, modules, and docs).
Out of Scope Changes check ✅ Passed All changes are directly related to the stated objectives: docstrings, tests, failure classification, and documentation updates. No extraneous changes detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
discord_activity_tracker/tests/test_task_markdown_coverage.py (1)

153-164: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert the auto-commit contract in the auto-commit test.

This test currently only verifies “no exception.” It should assert that export_and_push was called with auto_commit=True to actually protect the intended behavior.

Suggested assertion hardening
-    with patch(
+    with patch(
         "discord_activity_tracker.sync.export.export_and_push",
         return_value=True,
-    ):
+    ) as m:
         task_markdown_export_and_push(
             dry_run=False,
             skip_markdown_export=False,
             skip_remote_push=False,
             guild_id=srv.server_id,
             collector=collector,
         )
+    m.assert_called_once()
+    assert m.call_args.kwargs.get("auto_commit") is True
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@discord_activity_tracker/tests/test_task_markdown_coverage.py` around lines
153 - 164, The test currently only ensures task_markdown_export_and_push doesn't
raise; update it to assert the auto-commit contract by verifying the patched
export function (export.export_and_push) was called with auto_commit=True.
Locate the patch around "discord_activity_tracker.sync.export.export_and_push"
and after calling task_markdown_export_and_push add an assertion on the mock's
call args/kwargs (e.g., mock.assert_called_once_with or inspect mock.call_args)
to confirm auto_commit=True while preserving other expected args like guild_id,
collector, dry_run/skip flags.
🧹 Nitpick comments (1)
discord_activity_tracker/tests/test_sync_chat_exporter.py (1)

683-697: ⚡ Quick win

Consider asserting that embeds key is absent from the converted output.

The test verifies that content is preserved, but doesn't explicitly check that the embeds key has been removed from out. Adding assert "embeds" not in out would more directly validate the "ignored" behavior described in the test name.

📝 Suggested assertion
     out = convert_exporter_message_to_dict(raw, server_id=1, channel_id=2)
     assert out["content"] == "body"
+    assert "embeds" not in out
     from discord_activity_tracker.staging_schema import validate_normalized_message
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@discord_activity_tracker/tests/test_sync_chat_exporter.py` around lines 683 -
697, The test test_convert_exporter_message_with_embeds_key_ignored should
explicitly assert that the embeds key was removed by
convert_exporter_message_to_dict; add an assertion like assert "embeds" not in
out after calling convert_exporter_message_to_dict (before
validate_normalized_message) to ensure the converter ignores and drops the
embeds field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@core/errors.py`:
- Around line 195-209: The current branch only handles the exact exc_name
"HTTPException" and thus skips discord.py subclasses like Forbidden/NotFound
that also expose a status; change the gate so the status-based mapping runs for
any discord exception that exposes a status (e.g., replace the condition if
exc_name == "HTTPException": with a check like if hasattr(exc, "status") or if
status is not None after computing status = getattr(exc, "status", None)), then
keep the existing status -> CollectorFailureCategory mapping (RATE_LIMIT, AUTH,
NETWORK, UNKNOWN) so subclasses are classified correctly; reference the status
variable and the mapping logic in this block that returns
CollectorFailureCategory.

In `@discord_activity_tracker/tests/test_backfill_command_extra.py`:
- Line 81: The current assertion is too broad (assert "Failed" in out.getvalue()
or "Import complete" in out.getvalue()); change it to explicitly verify the
malformed-JSON path by asserting that the command output contains the specific
parse error message emitted by the backfill command (e.g., "Malformed JSON" or
"Failed to parse JSON") and does NOT contain the success summary "Import
complete". Update the assertion in the test (the failing line in
test_backfill_command_extra.py) to check for the exact error substring and
assert "Import complete" not in out.getvalue() so the test only passes when bad
JSON is correctly detected.

In `@discord_activity_tracker/tests/test_preprocessor_extra.py`:
- Around line 89-109: The test name misstates its intent: rename the test
function test_chain_to_document_short_content_returns_none to reflect it asserts
a non-None result (e.g., test_chain_to_document_long_content_returns_document)
or change the input to short content and assert None; specifically update the
test definition (the function name) or modify the root.content ("x" * 80) and
the assertion against _chain_to_document([root]) so the name matches the
behavior in the test.

In `@discord_activity_tracker/tests/test_run_command_coverage.py`:
- Around line 216-224: The test test_resolve_bounds_since_naive_becomes_utc is
using a UTC-aware timestamp ("...Z") instead of a naive one; update the call to
_resolve_exporter_date_bounds to pass a truly naive ISO timestamp (e.g.
"2026-04-01T00:00:00" without the Z) for the "since" value so the test verifies
naive→UTC normalization, and keep the existing assertions that after is not None
and after.tzinfo is not None while before remains None.
- Around line 167-180: Test currently only ensures no exceptions; update
test_handle_core_skip_pinecone_logs to patch "task_discord_sync" and
"task_markdown_export_and_push" as mocks (use patch(...) as mock_sync,
patch(...) as mock_md), call collector.cmd._handle_core(collector.options,
collector) with skip_pinecone=True, then assert mock_sync.assert_not_called() to
confirm Pinecone sync was skipped (you may also assert
mock_md.assert_called_once() if you want to ensure other tasks still ran).

In `@discord_activity_tracker/tests/test_sync_client.py`:
- Line 427: The test unpacks an unused binding named "inner" from
"mock_discord_pkg" (e.g., lines where you have "m, inner = mock_discord_pkg"),
which triggers lint RUF059; fix by removing the unused binding—either unpack to
a throwaway underscore ("m, _ = mock_discord_pkg") or only assign "m" (e.g., "m
= mock_discord_pkg[0]" or equivalent) wherever "inner" is not used (search for
occurrences of the variables "m", "inner", and "mock_discord_pkg" to update
them).

In `@discord_activity_tracker/tests/test_task_discord_sync_coverage.py`:
- Line 38: Ruff flags literal token strings in test fixtures (e.g.,
settings.DISCORD_USER_TOKEN and other similar assignments) as hardcoded secrets;
change those literal assignments to generate non-secret placeholder tokens at
runtime (for example, import secrets and assign settings.DISCORD_USER_TOKEN =
secrets.token_hex(16) or use a small pytest fixture that returns
secrets.token_hex(16)) so no raw string token appears in source; ensure you
update every occurrence referenced (the other token assignments noted in the
comment) to use the runtime-generated value and add the necessary import or
fixture to the test file.

---

Outside diff comments:
In `@discord_activity_tracker/tests/test_task_markdown_coverage.py`:
- Around line 153-164: The test currently only ensures
task_markdown_export_and_push doesn't raise; update it to assert the auto-commit
contract by verifying the patched export function (export.export_and_push) was
called with auto_commit=True. Locate the patch around
"discord_activity_tracker.sync.export.export_and_push" and after calling
task_markdown_export_and_push add an assertion on the mock's call args/kwargs
(e.g., mock.assert_called_once_with or inspect mock.call_args) to confirm
auto_commit=True while preserving other expected args like guild_id, collector,
dry_run/skip flags.

---

Nitpick comments:
In `@discord_activity_tracker/tests/test_sync_chat_exporter.py`:
- Around line 683-697: The test
test_convert_exporter_message_with_embeds_key_ignored should explicitly assert
that the embeds key was removed by convert_exporter_message_to_dict; add an
assertion like assert "embeds" not in out after calling
convert_exporter_message_to_dict (before validate_normalized_message) to ensure
the converter ignores and drops the embeds field.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6d33919b-68d8-4077-bd71-447b5003ccd8

📥 Commits

Reviewing files that changed from the base of the PR and between 3a674c5 and 980cde3.

📒 Files selected for processing (26)
  • core/errors.py
  • discord_activity_tracker/__init__.py
  • discord_activity_tracker/management/__init__.py
  • discord_activity_tracker/management/commands/__init__.py
  • discord_activity_tracker/management/commands/backfill_discord_activity_tracker.py
  • discord_activity_tracker/management/commands/run_discord_activity_tracker.py
  • discord_activity_tracker/models.py
  • discord_activity_tracker/services.py
  • discord_activity_tracker/sync/__init__.py
  • discord_activity_tracker/tests/test_backfill_command_extra.py
  • discord_activity_tracker/tests/test_chat_exporter_branch_coverage.py
  • discord_activity_tracker/tests/test_export_sync_coverage.py
  • discord_activity_tracker/tests/test_exporter_window.py
  • discord_activity_tracker/tests/test_failure_classification.py
  • discord_activity_tracker/tests/test_pinecone_runner_coverage.py
  • discord_activity_tracker/tests/test_preprocessor_extra.py
  • discord_activity_tracker/tests/test_run_command_coverage.py
  • discord_activity_tracker/tests/test_staging_schema_extra.py
  • discord_activity_tracker/tests/test_sync_chat_exporter.py
  • discord_activity_tracker/tests/test_sync_client.py
  • discord_activity_tracker/tests/test_task_discord_sync_coverage.py
  • discord_activity_tracker/tests/test_task_markdown_coverage.py
  • discord_activity_tracker/tests/test_workspace_clear_staging.py
  • discord_activity_tracker/tests/test_write_staging_json_schema_script.py
  • docs/Service_API.md
  • docs/service_api/discord_activity_tracker.md

Comment thread core/errors.py Outdated
Comment thread discord_activity_tracker/tests/test_backfill_command_extra.py Outdated
Comment thread discord_activity_tracker/tests/test_preprocessor_extra.py Outdated
Comment thread discord_activity_tracker/tests/test_run_command_coverage.py Outdated
Comment thread discord_activity_tracker/tests/test_run_command_coverage.py
Comment thread discord_activity_tracker/tests/test_sync_client.py Outdated
Comment thread discord_activity_tracker/tests/test_task_discord_sync_coverage.py Outdated
@leostar0412
Copy link
Copy Markdown
Collaborator Author

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 13, 2026

✅ Actions performed

Full review triggered.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (5)
discord_activity_tracker/tests/test_failure_classification.py (1)

64-68: ⚡ Quick win

Add direct tests for remaining Discord AUTH-name branches.

classify_failure explicitly maps PrivilegedIntentsRequired and ClientException to AUTH, but this file currently only asserts LoginFailure.

🧪 Suggested test additions
+def test_discord_privileged_intents_required_is_auth():
+    cls = type("PrivilegedIntentsRequired", (Exception,), {})
+    cls.__module__ = "discord.errors"
+    assert classify_failure(cls()) is CollectorFailureCategory.AUTH
+
+
+def test_discord_client_exception_is_auth():
+    cls = type("ClientException", (Exception,), {})
+    cls.__module__ = "discord.errors"
+    assert classify_failure(cls()) is CollectorFailureCategory.AUTH
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@discord_activity_tracker/tests/test_failure_classification.py` around lines
64 - 68, The tests only cover LoginFailure but classify_failure also maps
PrivilegedIntentsRequired and ClientException to AUTH; add two new unit tests in
discord_activity_tracker/tests/test_failure_classification.py that mirror
test_discord_login_failure_is_auth: create dynamic exception classes named
"PrivilegedIntentsRequired" and "ClientException" (set their __module__ to
"discord.errors"), instantiate them and assert classify_failure(instance) is
CollectorFailureCategory.AUTH, referencing classify_failure and
CollectorFailureCategory to locate the logic under test.
discord_activity_tracker/tests/test_pinecone_runner_coverage.py (1)

12-28: ⚡ Quick win

Strengthen skip-path tests with explicit negative call assertions.

These tests currently pass on “no exception” only. Patch call_command and assert it is not called so skip behavior is actually enforced.

Proposed test hardening
 def test_task_discord_pinecone_sync_dry_run():
-    task_discord_pinecone_sync(dry_run=True)
+    with patch("discord_activity_tracker.pinecone_runner.call_command") as cc:
+        task_discord_pinecone_sync(dry_run=True)
+    cc.assert_not_called()
@@
 def test_task_discord_pinecone_sync_skips_when_app_type_empty(monkeypatch, settings):
     monkeypatch.setattr(settings, "PINECONE_DISCORD_APP_TYPE", "")
     monkeypatch.setattr(settings, "PINECONE_DISCORD_NAMESPACE", "ns")
-    task_discord_pinecone_sync(dry_run=False)
+    with patch("discord_activity_tracker.pinecone_runner.call_command") as cc:
+        task_discord_pinecone_sync(dry_run=False)
+    cc.assert_not_called()
@@
 def test_task_discord_pinecone_sync_skips_when_namespace_empty(monkeypatch, settings):
     monkeypatch.setattr(settings, "PINECONE_DISCORD_APP_TYPE", "app")
     monkeypatch.setattr(settings, "PINECONE_DISCORD_NAMESPACE", "  ")
-    task_discord_pinecone_sync(dry_run=False)
+    with patch("discord_activity_tracker.pinecone_runner.call_command") as cc:
+        task_discord_pinecone_sync(dry_run=False)
+    cc.assert_not_called()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@discord_activity_tracker/tests/test_pinecone_runner_coverage.py` around lines
12 - 28, The skip-path tests for task_discord_pinecone_sync are brittle because
they only ensure no exception; patch django.core.management.call_command (or
monkeypatch the call_command symbol used by task_discord_pinecone_sync) in the
two tests test_task_discord_pinecone_sync_skips_when_app_type_empty and
test_task_discord_pinecone_sync_skips_when_namespace_empty, run
task_discord_pinecone_sync(dry_run=False), and assert that the patched
call_command was not called (e.g., using a spy/mock with assert_not_called) to
explicitly verify the skip behavior.
discord_activity_tracker/tests/test_task_discord_sync_coverage.py (1)

41-79: ⚡ Quick win

Make early-return tests assert that sync side effects are skipped.

The current assertions only verify “no crash.” Patch exporter/persist entry points and assert they are not invoked in skip_discord_sync and dry_run paths.

Proposed assertion upgrade
 def test_task_discord_sync_skip_returns_early(settings):
@@
-    task_discord_sync(
-        dry_run=False,
-        skip_discord_sync=True,
-        user_token=tok,
-        guild_id=1,
-        channel_ids=[],
-        after_date=None,
-        before_date=None,
-        collector=collector,
-    )
+    with patch(
+        "discord_activity_tracker.management.commands.run_discord_activity_tracker.export_guild_to_json"
+    ) as export_mock:
+        task_discord_sync(
+            dry_run=False,
+            skip_discord_sync=True,
+            user_token=tok,
+            guild_id=1,
+            channel_ids=[],
+            after_date=None,
+            before_date=None,
+            collector=collector,
+        )
+    export_mock.assert_not_called()
@@
 def test_task_discord_sync_dry_run_returns_early(settings):
@@
-    task_discord_sync(
-        dry_run=True,
-        skip_discord_sync=False,
-        user_token=tok,
-        guild_id=1,
-        channel_ids=[],
-        after_date=None,
-        before_date=None,
-        collector=collector,
-    )
+    with patch(
+        "discord_activity_tracker.management.commands.run_discord_activity_tracker.export_guild_to_json"
+    ) as export_mock:
+        task_discord_sync(
+            dry_run=True,
+            skip_discord_sync=False,
+            user_token=tok,
+            guild_id=1,
+            channel_ids=[],
+            after_date=None,
+            before_date=None,
+            collector=collector,
+        )
+    export_mock.assert_not_called()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@discord_activity_tracker/tests/test_task_discord_sync_coverage.py` around
lines 41 - 79, Update these two tests so they assert sync side effects are
skipped by patching/mocking the exporter/persist entry points that
task_discord_sync would call and asserting they were NOT invoked; locate
task_discord_sync and the DiscordActivityCollector instantiation in the tests,
inject mocks for the collector's export/persist callables (or the module-level
exporter.persist functions that task_discord_sync uses) before calling
task_discord_sync, then assert the mocked methods (e.g., export(), persist(), or
whatever entry-point names task_discord_sync invokes) have zero calls for both
skip_discord_sync and dry_run cases.
discord_activity_tracker/tests/test_run_command_coverage.py (1)

109-164: ⚡ Quick win

Isolate Pinecone side effects in tests not targeting Pinecone behavior.

Patch task_discord_pinecone_sync (or set skip_pinecone=True) in these orchestration tests to avoid environment-dependent behavior when Pinecone settings are present.

Also applies to: 237-252

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@discord_activity_tracker/tests/test_run_command_coverage.py` around lines 109
- 164, The orchestration tests call core tasks but don't isolate Pinecone side
effects; update the tests (e.g., test_handle_core_task_sync_skips_markdown,
test_handle_core_task_export_skips_sync,
test_handle_core_non_dry_calls_sync_and_markdown and the similar block at
237-252) to either patch
"discord_activity_tracker.management.commands.run_discord_activity_tracker.task_discord_pinecone_sync"
or set collector.options["skip_pinecone"]=True before invoking
collector.cmd._handle_core so Pinecone behavior is mocked out and the tests no
longer depend on Pinecone environment settings.
discord_activity_tracker/tests/test_task_markdown_coverage.py (1)

17-48: ⚡ Quick win

Add explicit assertions for branch intent in no-op/flag paths.

These tests currently pass on “no exception” only. Please assert observable behavior (e.g., export_and_push not called for skip/no-context, and called with expected flags for auto-commit) so regressions are caught.

Also applies to: 141-164

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@discord_activity_tracker/tests/test_task_markdown_coverage.py` around lines
17 - 48, The tests only check for no exceptions; update them to assert the
intended branch behavior by mocking/spy-ing the collector/export function: in
test_task_markdown_skip_export replace or attach a MagicMock to
DiscordActivityCollector.export_and_push and assert it was not called when
skip_markdown_export=True; in test_task_markdown_no_context_path similarly
assert export_and_push is not called when DISCORD_CONTEXT_REPO_PATH is empty;
also add assertions in the other referenced tests (lines ~141-164) to verify
export_and_push is called with the expected flags (e.g., auto_commit True/False)
when not skipped by checking the mock call args. Ensure you reference the
collector instance used in the tests and use mock.assert_not_called() /
mock.assert_called_once_with(...) to validate behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@discord_activity_tracker/tests/test_sync_chat_exporter.py`:
- Around line 683-697: The test
test_convert_exporter_message_with_embeds_key_ignored should explicitly assert
that embeds are dropped: after calling convert_exporter_message_to_dict(raw,
server_id=1, channel_id=2) and validating with validate_normalized_message(out,
source="embed-test"), add an assertion referencing the same output (out) that
verifies embeds is not present or is empty (e.g., assert "embeds" not in out or
out.get("embeds") in (None, [], {})) so any regression that preserves the embeds
key will fail.

---

Nitpick comments:
In `@discord_activity_tracker/tests/test_failure_classification.py`:
- Around line 64-68: The tests only cover LoginFailure but classify_failure also
maps PrivilegedIntentsRequired and ClientException to AUTH; add two new unit
tests in discord_activity_tracker/tests/test_failure_classification.py that
mirror test_discord_login_failure_is_auth: create dynamic exception classes
named "PrivilegedIntentsRequired" and "ClientException" (set their __module__ to
"discord.errors"), instantiate them and assert classify_failure(instance) is
CollectorFailureCategory.AUTH, referencing classify_failure and
CollectorFailureCategory to locate the logic under test.

In `@discord_activity_tracker/tests/test_pinecone_runner_coverage.py`:
- Around line 12-28: The skip-path tests for task_discord_pinecone_sync are
brittle because they only ensure no exception; patch
django.core.management.call_command (or monkeypatch the call_command symbol used
by task_discord_pinecone_sync) in the two tests
test_task_discord_pinecone_sync_skips_when_app_type_empty and
test_task_discord_pinecone_sync_skips_when_namespace_empty, run
task_discord_pinecone_sync(dry_run=False), and assert that the patched
call_command was not called (e.g., using a spy/mock with assert_not_called) to
explicitly verify the skip behavior.

In `@discord_activity_tracker/tests/test_run_command_coverage.py`:
- Around line 109-164: The orchestration tests call core tasks but don't isolate
Pinecone side effects; update the tests (e.g.,
test_handle_core_task_sync_skips_markdown,
test_handle_core_task_export_skips_sync,
test_handle_core_non_dry_calls_sync_and_markdown and the similar block at
237-252) to either patch
"discord_activity_tracker.management.commands.run_discord_activity_tracker.task_discord_pinecone_sync"
or set collector.options["skip_pinecone"]=True before invoking
collector.cmd._handle_core so Pinecone behavior is mocked out and the tests no
longer depend on Pinecone environment settings.

In `@discord_activity_tracker/tests/test_task_discord_sync_coverage.py`:
- Around line 41-79: Update these two tests so they assert sync side effects are
skipped by patching/mocking the exporter/persist entry points that
task_discord_sync would call and asserting they were NOT invoked; locate
task_discord_sync and the DiscordActivityCollector instantiation in the tests,
inject mocks for the collector's export/persist callables (or the module-level
exporter.persist functions that task_discord_sync uses) before calling
task_discord_sync, then assert the mocked methods (e.g., export(), persist(), or
whatever entry-point names task_discord_sync invokes) have zero calls for both
skip_discord_sync and dry_run cases.

In `@discord_activity_tracker/tests/test_task_markdown_coverage.py`:
- Around line 17-48: The tests only check for no exceptions; update them to
assert the intended branch behavior by mocking/spy-ing the collector/export
function: in test_task_markdown_skip_export replace or attach a MagicMock to
DiscordActivityCollector.export_and_push and assert it was not called when
skip_markdown_export=True; in test_task_markdown_no_context_path similarly
assert export_and_push is not called when DISCORD_CONTEXT_REPO_PATH is empty;
also add assertions in the other referenced tests (lines ~141-164) to verify
export_and_push is called with the expected flags (e.g., auto_commit True/False)
when not skipped by checking the mock call args. Ensure you reference the
collector instance used in the tests and use mock.assert_not_called() /
mock.assert_called_once_with(...) to validate behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 656a2dc3-ec6d-4613-93e3-226996210ae6

📥 Commits

Reviewing files that changed from the base of the PR and between 3a674c5 and ed3135f.

📒 Files selected for processing (26)
  • core/errors.py
  • discord_activity_tracker/__init__.py
  • discord_activity_tracker/management/__init__.py
  • discord_activity_tracker/management/commands/__init__.py
  • discord_activity_tracker/management/commands/backfill_discord_activity_tracker.py
  • discord_activity_tracker/management/commands/run_discord_activity_tracker.py
  • discord_activity_tracker/models.py
  • discord_activity_tracker/services.py
  • discord_activity_tracker/sync/__init__.py
  • discord_activity_tracker/tests/test_backfill_command_extra.py
  • discord_activity_tracker/tests/test_chat_exporter_branch_coverage.py
  • discord_activity_tracker/tests/test_export_sync_coverage.py
  • discord_activity_tracker/tests/test_exporter_window.py
  • discord_activity_tracker/tests/test_failure_classification.py
  • discord_activity_tracker/tests/test_pinecone_runner_coverage.py
  • discord_activity_tracker/tests/test_preprocessor_extra.py
  • discord_activity_tracker/tests/test_run_command_coverage.py
  • discord_activity_tracker/tests/test_staging_schema_extra.py
  • discord_activity_tracker/tests/test_sync_chat_exporter.py
  • discord_activity_tracker/tests/test_sync_client.py
  • discord_activity_tracker/tests/test_task_discord_sync_coverage.py
  • discord_activity_tracker/tests/test_task_markdown_coverage.py
  • discord_activity_tracker/tests/test_workspace_clear_staging.py
  • discord_activity_tracker/tests/test_write_staging_json_schema_script.py
  • docs/Service_API.md
  • docs/service_api/discord_activity_tracker.md

Comment thread discord_activity_tracker/tests/test_sync_chat_exporter.py
@leostar0412 leostar0412 requested a review from jonathanMLDev May 13, 2026 21:43
@leostar0412 leostar0412 requested a review from wpak-ai May 14, 2026 02:49
@wpak-ai wpak-ai merged commit 77bd37a into cppalliance:develop May 14, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Docstrings to Discord Tracker Module Add Test Coverage for Discord Tracker Module

3 participants