diff --git a/bot/code_review_bot/tasks/default.py b/bot/code_review_bot/tasks/default.py index 29ea244e3..d43856b73 100644 --- a/bot/code_review_bot/tasks/default.py +++ b/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 ] diff --git a/bot/code_review_bot/tasks/docupload.py b/bot/code_review_bot/tasks/docupload.py index 10dda63fa..978fd796f 100644 --- a/bot/code_review_bot/tasks/docupload.py +++ b/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): diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py index 2655f6fd0..de6359597 100644 --- a/bot/code_review_bot/workflow.py +++ b/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) diff --git a/bot/tests/test_workflow.py b/bot/tests/test_workflow.py index 0fdb96e04..51d8a699f 100644 --- a/bot/tests/test_workflow.py +++ b/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