diff --git a/bot/code_review_bot/__init__.py b/bot/code_review_bot/__init__.py index 7a99afde1..47c06447d 100644 --- a/bot/code_review_bot/__init__.py +++ b/bot/code_review_bot/__init__.py @@ -11,6 +11,9 @@ import requests import structlog + +# Workaround https://github.com/taskcluster/taskcluster/issues/9172 +import taskcluster.download from libmozdata.phabricator import LintResult, UnitResult, UnitResultState from taskcluster.helper import TaskclusterConfig diff --git a/bot/code_review_bot/analysis.py b/bot/code_review_bot/analysis.py index e6b33427d..f859b2d3a 100644 --- a/bot/code_review_bot/analysis.py +++ b/bot/code_review_bot/analysis.py @@ -1,3 +1,4 @@ +import enum from functools import cached_property import structlog @@ -17,6 +18,10 @@ ) +class AnalysisMode(enum.Enum): + Lint = 1 + + class PhabricatorRevisionBuild(PhabricatorBuild): """ Convert the bot revision into a libmozevent compatible build diff --git a/bot/code_review_bot/cli.py b/bot/code_review_bot/cli.py index 73e00a412..e86eb1798 100644 --- a/bot/code_review_bot/cli.py +++ b/bot/code_review_bot/cli.py @@ -16,6 +16,7 @@ UnitResult, UnitResultState, ) +from taskcluster.download import downloadArtifactToBuf from code_review_bot import ( AnalysisException, @@ -24,6 +25,7 @@ stats, taskcluster, ) +from code_review_bot.analysis import AnalysisMode from code_review_bot.config import settings from code_review_bot.report import get_reporters from code_review_bot.revisions import PhabricatorRevision, Revision @@ -171,6 +173,19 @@ def main(): # We need Phabricator API to list black-listed users settings.load_user_blacklist(taskcluster.secrets["user_blacklist"], phabricator_api) + # Run workflow according to source + w = Workflow( + reporters, + index_service, + queue_service, + phabricator_api, + taskcluster.secrets["ZERO_COVERAGE_ENABLED"], + # Update build status only when phabricator reporting is enabled + update_build=phabricator_reporting_enabled, + task_failures_ignored=taskcluster.secrets["task_failures_ignored"], + ) + + revision = None # Load unique revision try: if settings.generic_group_id: @@ -178,6 +193,7 @@ def main(): revision = PhabricatorRevision.from_decision_task( queue_service.task(settings.generic_group_id), phabricator_api ) + w.ingest_revision(revision, settings.generic_group_id) elif settings.phabricator_build_target: # Only Phabricator revisions is supported from build target revision = PhabricatorRevision.from_phabricator_trigger( @@ -186,13 +202,30 @@ def main(): ) if revision is None: return 0 + w.start_analysis(revision, settings.analysis_mode) else: + decision_task = queue_service.task(settings.try_group_id) + rawParams, _ = downloadArtifactToBuf( + taskId=settings.try_group_id, + name="public/parameters.yml", + queueService=queue_service, + ) + parameters = yaml.safe_load(bytes(rawParams)) revision = Revision.from_try_task( - queue_service.task(settings.try_task_id), - queue_service.task(settings.try_group_id), + decision_task, + parameters["phabricator_diff"], phabricator_api, ) + analysis_mode = None + if parameters["target_tasks_method"] == "codereview": + analysis_mode = AnalysisMode.Lint + + if not analysis_mode: + raise Exception("Cannot detect analysis mode; cannot proceed!") + + w.run(revision, analysis_mode) + except InvalidTrigger as e: logger.info("Early stop analysis due to invalid trigger", error=str(e)) @@ -204,40 +237,22 @@ def main(): # Stop cleanly as we just want to ignore that case, but report on sentry through warning return 0 except Exception as e: - # Report revision loading failure on production only - # On testing or dev instances, we can use different Phabricator - # configuration that do not match all the pulse messages sent - if settings.on_production: - raise - - else: - logger.info( - "Failed to load revision", - task=settings.try_task_id, - error=str(e), - phabricator=phabricator["url"], - ) - return 1 + if not revision: + # Report revision loading failure on production only + # On testing or dev instances, we can use different Phabricator + # configuration that do not match all the pulse messages sent + if settings.on_production: + raise + + else: + logger.info( + "Failed to load revision", + task=settings.try_task_id, + error=str(e), + phabricator=phabricator["url"], + ) + return 1 - # Run workflow according to source - w = Workflow( - reporters, - index_service, - queue_service, - phabricator_api, - taskcluster.secrets["ZERO_COVERAGE_ENABLED"], - # Update build status only when phabricator reporting is enabled - update_build=phabricator_reporting_enabled, - task_failures_ignored=taskcluster.secrets["task_failures_ignored"], - ) - try: - if settings.generic_group_id: - w.ingest_revision(revision, settings.generic_group_id) - elif settings.phabricator_build_target: - w.start_analysis(revision) - else: - w.run(revision) - except Exception as e: # Log errors to papertrail logger.error( "Static analysis failure", revision=revision, error=e, exc_info=True diff --git a/bot/code_review_bot/config.py b/bot/code_review_bot/config.py index 8fe6aeb25..73a98c398 100644 --- a/bot/code_review_bot/config.py +++ b/bot/code_review_bot/config.py @@ -14,6 +14,8 @@ import structlog +from code_review_bot.analysis import AnalysisMode + REPO_MOZILLA_CENTRAL = "https://hg.mozilla.org/mozilla-central" REPO_AUTOLAND = "https://hg.mozilla.org/integration/autoland" @@ -47,6 +49,7 @@ def __init__(self): self.try_group_id = None self.generic_group_id = None self.phabricator_build_target = None + self.analysis_mode = None self.repositories = [] self.decision_env_prefixes = [] @@ -90,8 +93,9 @@ def setup( taskcluster_parallel_requests=None, ): # Detect source from env - if "TRY_TASK_ID" in os.environ and "TRY_TASK_GROUP_ID" in os.environ: + if "TRY_TASK_ID" in os.environ: self.try_task_id = os.environ["TRY_TASK_ID"] + if "TRY_TASK_GROUP_ID" in os.environ: self.try_group_id = os.environ["TRY_TASK_GROUP_ID"] elif "GENERIC_TASK_GROUP_ID" in os.environ: self.generic_group_id = os.environ["GENERIC_TASK_GROUP_ID"] @@ -101,6 +105,10 @@ def setup( assert self.phabricator_build_target.startswith( "PHID-HMBT" ), f"Not a phabrication build target PHID: {self.phabricator_build_target}" + # TODO: remove the default after we can be certain it will always be + # present + mode = os.environ.get("ANALYSIS_MODE", "Lint") + self.analysis_mode = AnalysisMode[mode] else: raise Exception("Only TRY mode is supported") diff --git a/bot/code_review_bot/revisions/base.py b/bot/code_review_bot/revisions/base.py index e59e70290..b75e6cdad 100644 --- a/bot/code_review_bot/revisions/base.py +++ b/bot/code_review_bot/revisions/base.py @@ -275,7 +275,9 @@ def serialize(self): raise NotImplementedError @staticmethod - def from_try_task(try_task: dict, decision_task: dict, phabricator: PhabricatorAPI): + def from_try_task( + decision_task: dict, build_target_phid: str, phabricator: PhabricatorAPI + ): """ Load identifiers from Phabricator or Github, using the remote task description """ @@ -294,5 +296,5 @@ def from_try_task(try_task: dict, decision_task: dict, phabricator: PhabricatorA ) else: return PhabricatorRevision.from_try_task( - try_task["extra"]["code-review"], decision_task, phabricator + decision_task, build_target_phid, phabricator ) diff --git a/bot/code_review_bot/revisions/phabricator.py b/bot/code_review_bot/revisions/phabricator.py index 4b5d5cef1..70a37a820 100644 --- a/bot/code_review_bot/revisions/phabricator.py +++ b/bot/code_review_bot/revisions/phabricator.py @@ -133,15 +133,11 @@ def __str__(self): @staticmethod def from_try_task( - code_review: dict, decision_task: dict, phabricator: PhabricatorAPI + decision_task: dict, build_target_phid: str, phabricator: PhabricatorAPI ): """ Load identifiers from Phabricator, using the remote task description """ - # Load build target phid from the task env - build_target_phid = code_review.get("phabricator-diff") or code_review.get( - "phabricator-build-target" - ) assert ( build_target_phid is not None ), "Missing phabricator-build-target or phabricator-diff declaration" diff --git a/bot/code_review_bot/vcs.py b/bot/code_review_bot/vcs.py index 3dd5d9d8e..f669be19c 100644 --- a/bot/code_review_bot/vcs.py +++ b/bot/code_review_bot/vcs.py @@ -162,7 +162,7 @@ def apply_build(self, build): logger.info("Applying patch", phid=patch.phid, message=message) self.apply_patch(patch, message, commit) - def add_try_commit(self, build): + def add_try_commit(self, build, extra_parameters): """ Build and commit the file configuring try with try_task_config.json and the code-review workflow parameters in JSON @@ -171,11 +171,11 @@ def add_try_commit(self, build): config = { "version": 2, "parameters": { - "target_tasks_method": "codereview", - "optimize_target_tasks": True, "phabricator_diff": build.target_phid, }, } + config["parameters"].update(extra_parameters) + diff_phid = build.stack[-1].phid if build.revision_url: @@ -213,7 +213,7 @@ def __init__( ): self.skippable_files = skippable_files - def run(self, repository, build): + def run(self, repository, build, extra_parameters): """ Apply the stack of patches from the build, handling retries in case of try server errors @@ -228,7 +228,7 @@ def run(self, repository, build): ) try: - return self.handle_build(repository, build) + return self.handle_build(repository, build, extra_parameters) except RetryNeeded: build.retries += 1 @@ -289,7 +289,7 @@ def format_error(self, error): """Extract a readable error log from a VCS exception""" raise NotImplementedError - def handle_build(self, repository, build): + def handle_build(self, repository, build, extra_parameters): """ Try to load and apply a diff on local clone If successful, push to try and send a treeherder link @@ -321,7 +321,7 @@ def handle_build(self, repository, build): ) # Configure the try task - repository.add_try_commit(build) + repository.add_try_commit(build, extra_parameters) # Then push that stack on try tip = repository.push_to_try() diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py index 38a3f262d..572421cd4 100644 --- a/bot/code_review_bot/workflow.py +++ b/bot/code_review_bot/workflow.py @@ -16,6 +16,7 @@ from code_review_bot import Level, stats from code_review_bot.analysis import ( + AnalysisMode, PhabricatorRevisionBuild, publish_analysis_lando, publish_analysis_phabricator, @@ -104,10 +105,14 @@ def __init__( # Is local clone already setup ? self.clone_available = False - def run(self, revision): + def run(self, revision, analysis_mode: AnalysisMode): """ Find all issues on remote tasks and publish them """ + if analysis_mode == AnalysisMode.Lint: + return self._run_lint(revision) + + def _run_lint(self, revision): # Index ASAP Taskcluster task for this revision self.index(revision, state="started") @@ -273,7 +278,9 @@ def _build_tasks(tasks): # Publish issues in the backend self.backend_api.publish_issues(issues, revision) - def start_analysis(self, revision: PhabricatorRevision) -> None: + def start_analysis( + self, revision: PhabricatorRevision, analysis_mode: AnalysisMode + ): """ Apply a patch on a local clone and push to try to trigger a new Code review analysis """ @@ -365,9 +372,18 @@ def start_analysis(self, revision: PhabricatorRevision) -> None: # We'll clone the required repository repository.clone() + parameters = {} + if analysis_mode == AnalysisMode.Lint: + parameters.update( + { + "target_tasks_method": "codereview", + "optimize_target_tasks": True, + } + ) + # Apply the stack of patches and push to try worker = MercurialWorker() - output = worker.run(repository, build) + output = worker.run(repository, build, parameters) # Cancel any in-progress tasks from an earlier update # This is done after pushing to try to avoid delaying runs of the diff --git a/bot/tests/conftest.py b/bot/tests/conftest.py index 9faff79f2..8a1a7e98b 100644 --- a/bot/tests/conftest.py +++ b/bot/tests/conftest.py @@ -381,14 +381,6 @@ def mock_github(mock_config): ) -@pytest.fixture -def mock_try_task(): - """ - Mock a remote Try task definition - """ - return {"extra": {"code-review": {"phabricator-diff": "PHID-HMBT-test"}}} - - @pytest.fixture def mock_github_decision_task(): """ @@ -446,14 +438,14 @@ def mock_autoland_task(): @pytest.fixture -def mock_revision(mock_phabricator, mock_try_task, mock_decision_task, mock_config): +def mock_revision(mock_phabricator, mock_decision_task, mock_config): """ Mock a mercurial revision """ from code_review_bot.revisions import PhabricatorRevision, Revision with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) return revision @@ -470,15 +462,13 @@ def mock_revision_autoland(mock_phabricator, mock_autoland_task): @pytest.fixture -def mock_github_revision( - mock_github, mock_try_task, mock_github_decision_task, mock_config -): +def mock_github_revision(mock_github, mock_github_decision_task, mock_config): """ Mock a github revision """ from code_review_bot.revisions import GithubRevision, Revision - revision = Revision.from_try_task(mock_try_task, mock_github_decision_task, None) + revision = Revision.from_try_task(mock_github_decision_task, "PHID-HMBT-test", None) assert isinstance(revision, GithubRevision) return revision diff --git a/bot/tests/test_default.py b/bot/tests/test_default.py index 404ab4377..bf0dc54fa 100644 --- a/bot/tests/test_default.py +++ b/bot/tests/test_default.py @@ -5,6 +5,7 @@ import pytest from code_review_bot import Level +from code_review_bot.analysis import AnalysisMode from code_review_bot.tasks.default import DefaultIssue, DefaultTask @@ -65,7 +66,7 @@ def test_parser(mock_workflow, mock_revision, mock_hgmo, mock_backend): mock_workflow.backend_api.publish_revision = lambda rev: {} mock_revision.id = 1337 - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 1 issue = issues.pop() diff --git a/bot/tests/test_index.py b/bot/tests/test_index.py index d0e0df4da..89c49dcde 100644 --- a/bot/tests/test_index.py +++ b/bot/tests/test_index.py @@ -26,7 +26,7 @@ def as_dict(self): return self._details -def test_taskcluster_index(mock_config, mock_workflow, mock_try_task): +def test_taskcluster_index(mock_config, mock_workflow): """ Test the Taskcluster indexing API by mocking an online taskcluster state @@ -188,7 +188,6 @@ def test_index_phabricator( def test_index_from_try( mock_phabricator, phab, - mock_try_task, mock_decision_task, mock_workflow, mock_config, @@ -199,7 +198,7 @@ def test_index_from_try( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) mock_workflow.index_service = mock.Mock() diff --git a/bot/tests/test_mercurial.py b/bot/tests/test_mercurial.py index 890ce892f..28b215153 100644 --- a/bot/tests/test_mercurial.py +++ b/bot/tests/test_mercurial.py @@ -73,6 +73,12 @@ def test_robustcheckout(monkeypatch): ] +LINT_EXTRA_PARAMS = { + "optimize_target_tasks": True, + "target_tasks_method": "codereview", +} + + def test_push_to_try(PhabricatorMock, mock_mc, responses): """ Run mercurial worker on a single diff @@ -108,7 +114,7 @@ def test_push_to_try(PhabricatorMock, mock_mc, responses): ) worker = mercurial.MercurialWorker() - result = worker.run(mock_mc, build) + result = worker.run(mock_mc, build, LINT_EXTRA_PARAMS) # Check the treeherder link was generated tip = mock_mc.repo.tip() @@ -209,7 +215,7 @@ def _readme(content): assert not os.path.exists(config) worker = mercurial.MercurialWorker() - mode, out_build, details = worker.run(mock_mc, build) + mode, out_build, details = worker.run(mock_mc, build, LINT_EXTRA_PARAMS) # Check the treeherder link was queued tip = mock_mc.repo.tip() @@ -315,7 +321,7 @@ def test_dont_push_skippable_files_to_try(PhabricatorMock, mock_mc): worker = mercurial.MercurialWorker( skippable_files=["test.txt"], ) - mode, out_build, details = worker.run(mock_mc, build) + mode, out_build, details = worker.run(mock_mc, build, LINT_EXTRA_PARAMS) # Check the treeherder link was queued tip = mock_mc.repo.tip() @@ -386,7 +392,7 @@ def test_treeherder_link(PhabricatorMock, mock_mc): ) worker = mercurial.MercurialWorker() - mode, out_build, details = worker.run(mock_mc, build) + mode, out_build, details = worker.run(mock_mc, build, LINT_EXTRA_PARAMS) # Check the treeherder link was queued tip = mock_mc.repo.tip() @@ -436,7 +442,7 @@ def boom(*args): mock_mc.apply_build = boom worker = mercurial.MercurialWorker() - mode, out_build, details = worker.run(mock_mc, build) + mode, out_build, details = worker.run(mock_mc, build, LINT_EXTRA_PARAMS) # Check the unit result was published @@ -485,7 +491,7 @@ def test_failure_mercurial(PhabricatorMock, mock_config, mock_mc): ) worker = mercurial.MercurialWorker() - mode, out_build, details = worker.run(mock_mc, build) + mode, out_build, details = worker.run(mock_mc, build, LINT_EXTRA_PARAMS) # Check the treeherder link was queued assert mode == "fail:mercurial" @@ -558,7 +564,7 @@ def test_push_to_try_nss(PhabricatorMock, mock_nss): ) worker = mercurial.MercurialWorker() - mode, out_build, details = worker.run(mock_nss, build) + mode, out_build, details = worker.run(mock_nss, build, LINT_EXTRA_PARAMS) # Check the treeherder link was queued tip = mock_nss.repo.tip() @@ -638,7 +644,7 @@ def test_crash_utf8_author(PhabricatorMock, mock_mc): # Run the mercurial worker on that patch only worker = mercurial.MercurialWorker() - mode, out_build, details = worker.run(mock_mc, build) + mode, out_build, details = worker.run(mock_mc, build, LINT_EXTRA_PARAMS) # Check we have the patch with utf-8 author properly applied assert [(c.author, c.desc) for c in mock_mc.repo.log()] == [ @@ -708,7 +714,7 @@ def test_unexpected_push_failure(PhabricatorMock, mock_mc): repository_mock.retries = 0 worker = mercurial.MercurialWorker() - mode, out_build, details = worker.run(repository_mock, build) + mode, out_build, details = worker.run(repository_mock, build, LINT_EXTRA_PARAMS) assert mode == "success" assert out_build == build @@ -765,7 +771,7 @@ def test_push_failure_max_retries(PhabricatorMock, mock_mc, monkeypatch): ) worker = mercurial.MercurialWorker() - mode, out_build, details = worker.run(repository_mock, build) + mode, out_build, details = worker.run(repository_mock, build, LINT_EXTRA_PARAMS) # Check the treeherder link was queued assert build.retries == 3 @@ -836,7 +842,7 @@ def test_push_closed_try(PhabricatorMock, mock_mc, monkeypatch): worker = mercurial.MercurialWorker() - mode, out_build, details = worker.run(repository_mock, build) + mode, out_build, details = worker.run(repository_mock, build, LINT_EXTRA_PARAMS) assert repository_mock.push_to_try.call_count == 2 assert mode == "success" diff --git a/bot/tests/test_phabricator_analysis.py b/bot/tests/test_phabricator_analysis.py index 8aa95c31a..965048d94 100644 --- a/bot/tests/test_phabricator_analysis.py +++ b/bot/tests/test_phabricator_analysis.py @@ -12,6 +12,7 @@ from code_review_bot import mercurial from code_review_bot.analysis import ( + AnalysisMode, publish_analysis_phabricator, ) from code_review_bot.config import RepositoryConf @@ -116,7 +117,7 @@ def test_workflow_private_build( phabricator=api, ) - assert mock_workflow.start_analysis(revision) is None + assert mock_workflow.start_analysis(revision, AnalysisMode.Lint) is None # No clone nor push happened assert hgrun_calls == [] @@ -172,7 +173,7 @@ def mock_hgrun(cmd): phabricator=api, ) - mock_workflow.start_analysis(revision) + mock_workflow.start_analysis(revision, AnalysisMode.Lint) # Check hgrun initial call to clone mozilla central through robust checkout assert hgrun_calls == [ diff --git a/bot/tests/test_remote.py b/bot/tests/test_remote.py index 8af91b1a0..baa37007a 100644 --- a/bot/tests/test_remote.py +++ b/bot/tests/test_remote.py @@ -7,6 +7,7 @@ from libmozdata.phabricator import BuildState from code_review_bot import stats +from code_review_bot.analysis import AnalysisMode @pytest.fixture @@ -52,7 +53,7 @@ def test_no_deps( ) with pytest.raises(AssertionError) as e: - mock_workflow.run(mock_revision) + mock_workflow.run(mock_revision, AnalysisMode.Lint) assert str(e.value) == "No task dependencies to analyze" @@ -112,7 +113,7 @@ def test_baseline( }, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 2 issue = issues[0] @@ -168,7 +169,7 @@ def test_no_failed( "extra-task": {}, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 0 assert mock_revision._state == BuildState.Pass @@ -196,13 +197,13 @@ def test_no_issues( "extra-task": {}, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 0 assert mock_revision._state == BuildState.Fail # Now mark that task failure as ignorable mock_workflow.task_failures_ignored = ["source-test-mozlint-flake8"] - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 0 assert mock_revision._state == BuildState.Pass @@ -248,7 +249,7 @@ def test_build_status_fail_on_error( }, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 2 assert mock_revision._state == BuildState.Fail @@ -294,7 +295,7 @@ def test_build_status_pass_on_warning( }, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 2 assert mock_revision._state == BuildState.Pass @@ -320,7 +321,7 @@ def test_unsupported_analyzer( "extra-task": {}, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 0 assert mock_revision._state == BuildState.Pass @@ -357,7 +358,7 @@ def test_mozlint_task( }, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 1 issue = issues[0] assert isinstance(issue, MozLintIssue) @@ -428,7 +429,7 @@ def test_clang_tidy_task( }, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 2 issue = issues[0] assert isinstance(issue, ClangTidyIssue) @@ -509,7 +510,7 @@ def test_clang_format_task( } mock_workflow.setup_mock_tasks(tasks) assert len(mock_revision.improvement_patches) == 0 - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 1 issue = issues[0] assert isinstance(issue, ClangFormatIssue) @@ -576,7 +577,7 @@ def test_no_tasks( "remoteTryTask": {"dependencies": ["decision", "someOtherDockerbuild"]}, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 0 assert mock_revision._state == BuildState.Pass @@ -612,12 +613,12 @@ def test_zero_coverage_option( ) mock_workflow.zero_coverage_enabled = False - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 0 assert mock_revision._state == BuildState.Pass mock_workflow.zero_coverage_enabled = True - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 1 assert isinstance(issues[0], CoverageIssue) assert mock_revision._state == BuildState.Pass @@ -661,7 +662,7 @@ def test_external_tidy_task( }, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 1 issue = issues[0] assert isinstance(issue, ExternalTidyIssue) diff --git a/bot/tests/test_reporter_github.py b/bot/tests/test_reporter_github.py index 64465adb2..75bee8337 100644 --- a/bot/tests/test_reporter_github.py +++ b/bot/tests/test_reporter_github.py @@ -22,7 +22,6 @@ def test_github_review( mock_github, mock_config, phab, - mock_try_task, mock_github_decision_task, mock_task, mock_backend_secret, @@ -30,7 +29,7 @@ def test_github_review( """ Report 2 clang tidy issues by pushing a review to a Github pull request """ - revision = Revision.from_try_task(mock_try_task, mock_github_decision_task, None) + revision = Revision.from_try_task(mock_github_decision_task, "PHID-HMBT-test", None) assert isinstance(revision, GithubRevision) revision.lines = { # Add dummy lines diff @@ -169,13 +168,12 @@ def test_github_review_cleanup( mock_github, mock_config, phab, - mock_try_task, mock_github_decision_task, mock_task, mock_backend_secret, ): """In case no issue is found, previous reviews are dismissed""" - revision = Revision.from_try_task(mock_try_task, mock_github_decision_task, None) + revision = Revision.from_try_task(mock_github_decision_task, "PHID-HMBT-test", None) revision.lines = {} revision.files = ["test.txt", "test.cpp", "another_test.cpp"] revision.id = 52 diff --git a/bot/tests/test_reporter_phabricator.py b/bot/tests/test_reporter_phabricator.py index 2e242c78c..9a251e027 100644 --- a/bot/tests/test_reporter_phabricator.py +++ b/bot/tests/test_reporter_phabricator.py @@ -262,15 +262,13 @@ """ -def test_phabricator_clang_tidy( - mock_phabricator, phab, mock_try_task, mock_decision_task, mock_task -): +def test_phabricator_clang_tidy(mock_phabricator, phab, mock_decision_task, mock_task): """ Test Phabricator reporter publication on a mock clang-tidy issue """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -300,14 +298,14 @@ def test_phabricator_clang_tidy( def test_phabricator_clang_format( - mock_config, mock_phabricator, phab, mock_try_task, mock_decision_task, mock_task + mock_config, mock_phabricator, phab, mock_decision_task, mock_task ): """ Test Phabricator reporter publication on a mock clang-format issue """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -342,7 +340,7 @@ def test_phabricator_clang_format( def test_phabricator_mozlint( - mock_config, mock_phabricator, phab, mock_try_task, mock_decision_task, mock_task + mock_config, mock_phabricator, phab, mock_decision_task, mock_task ): """ Test Phabricator reporter publication on two mock mozlint issues @@ -350,7 +348,7 @@ def test_phabricator_mozlint( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -432,7 +430,6 @@ def test_phabricator_coverage( mock_config, mock_phabricator, phab, - mock_try_task, mock_decision_task, mock_task, ): @@ -440,7 +437,7 @@ def test_phabricator_coverage( Test Phabricator reporter publication on a mock coverage issue """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -493,7 +490,6 @@ def test_phabricator_no_coverage_on_deleted_file( mock_config, mock_phabricator, phab, - mock_try_task, mock_decision_task, mock_task, ): @@ -507,7 +503,7 @@ def raise_404(*args, **kwargs): raise HTTPError(response=resp_mock) with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -533,7 +529,6 @@ def test_phabricator_clang_tidy_and_coverage( mock_config, mock_phabricator, phab, - mock_try_task, mock_decision_task, mock_task, ): @@ -542,7 +537,7 @@ def test_phabricator_clang_tidy_and_coverage( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -654,7 +649,6 @@ def test_phabricator_analyzers( valid_patches, mock_config, mock_phabricator, - mock_try_task, mock_decision_task, mock_task, ): @@ -667,7 +661,7 @@ def test_phabricator_analyzers( api.comment = unittest.mock.Mock(return_value=True) # Always use the same setup, only varies the analyzers - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = {"test.cpp": [0, 41, 42, 43], "dom/test.cpp": [42]} revision.id = 52 @@ -743,7 +737,7 @@ def test_phabricator_analyzers( def test_phabricator_clang_tidy_build_error( - mock_phabricator, phab, mock_try_task, mock_decision_task, mock_task + mock_phabricator, phab, mock_decision_task, mock_task ): """ Test Phabricator Lint for a ClangTidyIssue with build error @@ -752,7 +746,7 @@ def test_phabricator_clang_tidy_build_error( from code_review_bot import Level with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -803,15 +797,13 @@ def test_phabricator_clang_tidy_build_error( assert phab.comments[51] == [VALID_BUILD_ERROR_MESSAGE] -def test_full_file( - mock_config, mock_phabricator, phab, mock_try_task, mock_decision_task, mock_task -): +def test_full_file(mock_config, mock_phabricator, phab, mock_decision_task, mock_task): """ Test Phabricator reporter supports an issue on a full file """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -868,13 +860,13 @@ def test_full_file( ] -def test_task_failures(mock_phabricator, phab, mock_try_task, mock_decision_task): +def test_task_failures(mock_phabricator, phab, mock_decision_task): """ Test Phabricator reporter publication with some task failures """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.id = 52 reporter = PhabricatorReporter({"analyzers": ["clang-tidy"]}, api=api) @@ -892,15 +884,13 @@ def test_task_failures(mock_phabricator, phab, mock_try_task, mock_decision_task assert phab.comments[51] == [VALID_TASK_FAILURES_MESSAGE] -def test_extra_errors( - mock_phabricator, mock_try_task, mock_decision_task, phab, mock_task -): +def test_extra_errors(mock_phabricator, mock_decision_task, phab, mock_task): """ Test Phabricator reporter publication with some errors outside of patch """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = {"path/to/file.py": [1, 2, 3]} revision.files = ["path/to/file.py"] @@ -986,13 +976,13 @@ def test_extra_errors( assert phab.comments[51] == [VALID_MOZLINT_MESSAGE] -def test_phabricator_notices(mock_phabricator, phab, mock_try_task, mock_decision_task): +def test_phabricator_notices(mock_phabricator, phab, mock_decision_task): """ Test Phabricator reporter publication on a mock clang-format issue """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -1035,13 +1025,13 @@ def test_phabricator_notices(mock_phabricator, phab, mock_try_task, mock_decisio ] -def test_phabricator_tgdiff(mock_phabricator, phab, mock_try_task, mock_decision_task): +def test_phabricator_tgdiff(mock_phabricator, phab, mock_decision_task): """ Test Phabricator reporter publication on a mock clang-format issue """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -1066,14 +1056,14 @@ def test_phabricator_tgdiff(mock_phabricator, phab, mock_try_task, mock_decision def test_phabricator_external_tidy( - mock_phabricator, phab, mock_try_task, mock_decision_task, mock_task + mock_phabricator, phab, mock_decision_task, mock_task ): """ Test Phabricator reporter publication on a mock external-tidy issue """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -1118,14 +1108,14 @@ def test_phabricator_external_tidy( def test_phabricator_newer_diff( - monkeypatch, mock_phabricator, phab, mock_try_task, mock_decision_task, mock_task + monkeypatch, mock_phabricator, phab, mock_decision_task, mock_task ): """ Test Phabricator reporter publication won't be called when a newer diff exists for the patch """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -1194,7 +1184,7 @@ def test_phabricator_newer_diff( def test_phabricator_former_diff_comparison( - monkeypatch, mock_phabricator, phab, mock_try_task, mock_decision_task, mock_task + monkeypatch, mock_phabricator, phab, mock_decision_task, mock_task ): """ Test Phabricator reporter publication shows the number of unresolved @@ -1204,7 +1194,7 @@ def test_phabricator_former_diff_comparison( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -1338,7 +1328,6 @@ def test_phabricator_before_after_comment( monkeypatch, mock_phabricator, phab, - mock_try_task, mock_decision_task, mock_task, mock_taskcluster_config, @@ -1353,7 +1342,7 @@ def test_phabricator_before_after_comment( mock_taskcluster_config.secrets = {"BEFORE_AFTER_RATIO": 1} with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff diff --git a/bot/tests/test_testing_policy.py b/bot/tests/test_testing_policy.py index a3c67dfbe..93b638b29 100644 --- a/bot/tests/test_testing_policy.py +++ b/bot/tests/test_testing_policy.py @@ -373,7 +373,7 @@ def test_api_failure_is_not_fatal(api, revision): def test_phabricator_reporter_sets_tag( - mock_phabricator, phab, mock_try_task, mock_decision_task, mock_backend_secret + mock_phabricator, phab, mock_decision_task, mock_backend_secret ): """ The Phabricator reporter sets the tag on a documentation-only revision @@ -382,7 +382,7 @@ def test_phabricator_reporter_sets_tag( from code_review_bot.revisions import Revision with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) revision.lines = {"docs/index.rst": [1, 2], "dom/base/test/test_foo.html": [3]} revision.files = list(revision.lines.keys()) revision.id = 52 @@ -397,7 +397,7 @@ def test_phabricator_reporter_sets_tag( def test_phabricator_reporter_skips_code_changes( - mock_phabricator, phab, mock_try_task, mock_decision_task, mock_backend_secret + mock_phabricator, phab, mock_decision_task, mock_backend_secret ): """ The Phabricator reporter does not tag a revision modifying code @@ -406,7 +406,7 @@ def test_phabricator_reporter_skips_code_changes( from code_review_bot.revisions import Revision with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) revision.lines = {"docs/index.rst": [1, 2], "dom/base/nsDocument.cpp": [3]} revision.files = list(revision.lines.keys()) revision.id = 52 diff --git a/bot/tests/test_workflow.py b/bot/tests/test_workflow.py index 29eb99f21..3f901a8fc 100644 --- a/bot/tests/test_workflow.py +++ b/bot/tests/test_workflow.py @@ -12,6 +12,7 @@ import responses from libmozdata.phabricator import ConduitError +from code_review_bot.analysis import AnalysisMode from code_review_bot.config import Settings, TaskCluster from code_review_bot.revisions import PhabricatorRevision from code_review_bot.tasks.clang_format import ClangFormatIssue, ClangFormatTask @@ -238,7 +239,7 @@ def test_before_after(mock_taskcluster_config, mock_workflow, mock_task, mock_re # Set backend ID as the publication is disabled for tests mock_revision.id = 1337 assert mock_revision.before_after_feature is True - mock_workflow.run(mock_revision) + mock_workflow.run(mock_revision, AnalysisMode.Lint) assert mock_workflow.publish.call_args_list == [ mock.call( mock_revision,