Make source-test-doc-upload tasks check both issues and upload files.

bot/code_review_bot/tasks/default.py

@@ -54,14 +54,23 @@ class DefaultTask(AnalysisTask):
     https://github.com/mozilla/code-review/blob/master/docs/analysis_format.md
     """
 
-    artifacts = ["public/code-review/issues.json"]
+    ISSUES_ARTIFACT = "public/code-review/issues.json"
+    artifacts = [ISSUES_ARTIFACT]
 
     def parse_issues(self, artifacts, revision):
         """
         Parse issues from a log file content
+
+        Only the dedicated code-review issues artifact is read, so that
+        subclasses can extend `artifacts` with other, differently shaped
+        artifacts (e.g. DocUploadTask) without breaking this parser.
         """
         assert isinstance(artifacts, dict)
 
+        issues_artifact = artifacts.get(self.ISSUES_ARTIFACT)
+        if not issues_artifact:
+            return []
+
         def default_check(issue):
             # Use analyzer name when check is not provided
             # This happens for analyzers who only have one rule
@@ -84,8 +93,7 @@ def default_check(issue):
                 check=default_check(issue),
                 message=issue["message"],
             )
-            for artifact in artifacts.values()
-            for _, path_issues in artifact.items()
+            for _, path_issues in issues_artifact.items()
             for issue in path_issues
         ]
 

bot/code_review_bot/tasks/docupload.py

@@ -3,6 +3,7 @@
 import structlog
 
 from code_review_bot.tasks.base import NoticeTask
+from code_review_bot.tasks.default import DefaultTask
 
 logger = structlog.get_logger(__name__)
 
@@ -58,12 +59,19 @@ def direct_doc_url(path, docs_url, trees):
     return docs_url
 
 
-class DocUploadTask(NoticeTask):
+class DocUploadTask(DefaultTask, NoticeTask):
     """
     Support doc-upload tasks
+
+    This task both reports issues using the generic code-review format
+    (inherited from DefaultTask, e.g. failure reporting) and builds a
+    documentation preview notice (NoticeTask).
     """
 
-    artifacts = ["public/firefox-source-docs-url.txt", "public/trees.json"]
+    artifacts = DefaultTask.artifacts + [
+        "public/firefox-source-docs-url.txt",
+        "public/trees.json",
+    ]
 
     @property
     def display_name(self):

bot/code_review_bot/workflow.py

@@ -641,7 +641,7 @@ def _in_group(dep_id):
                         for patch in task_patches:
                             revision.add_improvement_patch(task, patch)
 
-                    elif isinstance(task, NoticeTask):
+                    if isinstance(task, NoticeTask):
                         notice = task.build_notice(artifacts, revision)
                         if notice:
                             notices.append(notice)

bot/tests/test_workflow.py

@@ -16,6 +16,7 @@
 from code_review_bot.tasks.clang_format import ClangFormatIssue, ClangFormatTask
 from code_review_bot.tasks.clang_tidy import ClangTidyTask
 from code_review_bot.tasks.clang_tidy_external import ExternalTidyTask
+from code_review_bot.tasks.docupload import DocUploadTask
 from code_review_bot.tasks.lint import MozLintTask
 from code_review_bot.tasks.tgdiff import TaskGraphDiffTask
 
@@ -33,6 +34,8 @@
         ("source-test-mozlint-whatever", MozLintTask, True),
         ("source-test-clang-format", ClangFormatTask, False),
         ("source-test-clang-format", ClangFormatTask, True),
+        ("source-test-doc-upload", DocUploadTask, False),
+        ("source-test-doc-upload", DocUploadTask, True),
         ("source-test-taskgraph-diff", TaskGraphDiffTask, False),
         ("source-test-taskgraph-diff", TaskGraphDiffTask, True),
         ("source-test-unsupported", None, False),
@@ -67,6 +70,81 @@ def test_build_task(task_name, result, on_autoland, mock_config, mock_workflow):
         assert isinstance(task, result)
 
 
+def test_find_issues_doc_upload_both_issues_and_notice(mock_workflow, mock_revision):
+    """
+    A source-test-doc-upload task both reports issues found in its
+    code-review issues.json artifact, and builds a documentation preview
+    notice from its doc artifacts (DocUploadTask now supports both, see #3488).
+    """
+    mock_revision.files = ["docs/folderA/index.rst"]
+
+    mock_workflow.setup_mock_tasks(
+        {
+            "remoteTryTask": {"dependencies": ["doc-upload-task"]},
+            "doc-upload-task": {
+                "name": "source-test-doc-upload",
+                "artifacts": {
+                    "public/code-review/issues.json": {
+                        "docs/folderA/index.rst": [
+                            {
+                                "path": "docs/folderA/index.rst",
+                                "line": 1,
+                                "column": 1,
+                                "level": "warning",
+                                "check": "some-check",
+                                "message": "A doc issue",
+                            }
+                        ]
+                    },
+                    "public/firefox-source-docs-url.txt": "http://firefox-test-docs.mozilla.org/index.html",
+                    "public/trees.json": {"section1": "docs/folderA"},
+                },
+            },
+        }
+    )
+    # Bypass revision publication check (backend storage is disabled in tests)
+    mock_workflow.backend_api.publish_revision = lambda rev: {}
+
+    issues, task_failures, notices, reviewers = mock_workflow.find_issues(
+        mock_revision, "remoteTryTask"
+    )
+
+    assert len(issues) == 1
+    assert issues[0].message == "A doc issue"
+    assert task_failures == []
+    assert len(notices) == 1
+    assert "docs/folderA/index.rst" in notices[0]
+
+
+def test_find_issues_doc_upload_failure_reported_once(mock_workflow, mock_revision):
+    """
+    A failed source-test-doc-upload task with no parsed issues or patches
+    must only be reported once as a task failure, even though DocUploadTask
+    exercises both the AnalysisTask and NoticeTask dispatch branches.
+    """
+    mock_revision.files = []
+
+    mock_workflow.setup_mock_tasks(
+        {
+            "remoteTryTask": {"dependencies": ["doc-upload-task"]},
+            "doc-upload-task": {
+                "name": "source-test-doc-upload",
+                "state": "failed",
+                "artifacts": {},
+            },
+        }
+    )
+    mock_workflow.backend_api.publish_revision = lambda rev: {}
+
+    issues, task_failures, notices, reviewers = mock_workflow.find_issues(
+        mock_revision, "remoteTryTask"
+    )
+
+    assert issues == []
+    assert notices == []
+    assert len(task_failures) == 1
+
+
 def test_on_production(mock_config, mock_repositories):
     """
     Test the production environment detection