зеркало из https://github.com/mozilla/code-review.git
Compare issues with the backend (a.k.a. before/after) (#1581)
* Compare issues with known ones in the backend * rebase * Update list_repo_issues helper * Fix issues grouping by path * Cache Issue hash computed value * Repport issues that are tagged as new * Update publication * Add a comment section for issues outside the diff * Publish issues inside or outside the patch in two similar sections of the Phabricator comment * Only publish new issues to harbormaster * Improve message for known issues * Always refer to issues as 'defects' in Phabricator comment * Fix Phabricator publish return value * Make sure to run before/after on all the diffs of a revision * Only store new issues in bot's backend while runing before/after * Group in and out issue main comments * Preserve original publications (remove known issues footer) * Update existing tests * Add tests * Use a cached prop for issues' hash Define local repository in runtime settings * Fix before after random check for a revision * Suggestions * Use itertools.groupby or paths lookup * Improve RNG seed comment * Always compare issues that are found inside the diff * Raise error to sentry when GECKO_BASE_REV is unset on the decision task * Disable before/after for a non published revision * Override runtime local repository setting * Do not display existing issues stats
This commit is contained in:
Valentin Rigal
Andi-Bogdan Postelnicu
Родитель
593ef68629
Коммит
2e4e05cfc0
|
|
@ -8,12 +8,14 @@ import enum
|
|||
import hashlib
|
||||
import json
|
||||
import os
|
||||
from functools import cached_property
|
||||
|
||||
import requests
|
||||
import structlog
|
||||
from libmozdata.phabricator import LintResult, UnitResult, UnitResultState
|
||||
from taskcluster.helper import TaskclusterConfig
|
||||
|
||||
from code_review_bot.config import settings
|
||||
from code_review_bot.stats import InfluxDb
|
||||
from code_review_bot.tasks.base import AnalysisTask
|
||||
|
||||
|
|
@ -103,6 +105,10 @@ class Issue(abc.ABC):
|
|||
if self.fix is not None:
|
||||
assert self.language is not None, "Missing fix language"
|
||||
|
||||
# Mark the issue as known by default, so only errors are reported
|
||||
# The before/after feature may tag some issues as new, so they are reported
|
||||
self.new_issue = False
|
||||
|
||||
def __str__(self):
|
||||
line = f"line {self.line}" if self.line is not None else "full file"
|
||||
return f"{self.analyzer.name} issue {self.check}@{self.level.value} {self.path} {line}"
|
||||
|
|
@ -131,6 +137,10 @@ class Issue(abc.ABC):
|
|||
if not self.validates():
|
||||
return False
|
||||
|
||||
if self.revision.before_after_feature:
|
||||
# Only publish new issues or issues inside the diff
|
||||
return self.new_issue or self.in_patch
|
||||
|
||||
# An error is always published
|
||||
if self.level == Level.Error:
|
||||
return True
|
||||
|
|
@ -140,11 +150,16 @@ class Issue(abc.ABC):
|
|||
return self.on_backend["publishable"]
|
||||
|
||||
# Fallback to in_patch detection
|
||||
return self.in_patch
|
||||
|
||||
@property
|
||||
def in_patch(self):
|
||||
return self.revision.contains(self)
|
||||
|
||||
def build_hash(self, local_repository=None):
|
||||
@cached_property
|
||||
def hash(self):
|
||||
"""
|
||||
Build a unique hash identifying that issue
|
||||
Build a unique hash identifying that issue and cache the resulting value
|
||||
The text concerned by the issue is used and not its position in the file
|
||||
Message content is hashed as a single linter may return multiple issues on a single line
|
||||
We make the assumption that the message does not contain the line number
|
||||
|
|
@ -152,7 +167,9 @@ class Issue(abc.ABC):
|
|||
"""
|
||||
assert self.revision is not None, "Missing revision"
|
||||
|
||||
local_repository = settings.runtime.get("mercurial_repository")
|
||||
if local_repository:
|
||||
logger.debug("Using the local repository to build issue's hash")
|
||||
try:
|
||||
with open(local_repository / self.path) as f:
|
||||
file_content = f.read()
|
||||
|
|
@ -179,7 +196,8 @@ class Issue(abc.ABC):
|
|||
raise
|
||||
|
||||
if file_content is None:
|
||||
return None
|
||||
self._hash = None
|
||||
return self._hash
|
||||
|
||||
# Build raw content:
|
||||
# 1. lines affected by patch
|
||||
|
|
@ -242,16 +260,16 @@ class Issue(abc.ABC):
|
|||
"""
|
||||
raise NotImplementedError
|
||||
|
||||
def as_dict(self, issue_hash=None):
|
||||
def as_dict(self):
|
||||
"""
|
||||
Build the serializable dict representation of the issue
|
||||
Used by debugging tools
|
||||
"""
|
||||
if not issue_hash:
|
||||
try:
|
||||
issue_hash = self.build_hash()
|
||||
except Exception as e:
|
||||
logger.warn("Failed to build issue hash", error=str(e), issue=str(self))
|
||||
issue_hash = None
|
||||
try:
|
||||
issue_hash = self.hash
|
||||
except Exception as e:
|
||||
logger.warn("Failed to build issue hash", error=str(e), issue=str(self))
|
||||
|
||||
return {
|
||||
"analyzer": self.analyzer.name,
|
||||
|
|
@ -262,7 +280,7 @@ class Issue(abc.ABC):
|
|||
"check": self.check,
|
||||
"level": self.level.value,
|
||||
"message": self.message,
|
||||
"in_patch": self.revision.contains(self),
|
||||
"in_patch": self.in_patch,
|
||||
"validates": self.validates(),
|
||||
"publishable": self.is_publishable(),
|
||||
"hash": issue_hash,
|
||||
|
|
|
|||
|
|
@ -111,10 +111,9 @@ class BackendAPI(object):
|
|||
|
||||
return backend_revision
|
||||
|
||||
def publish_issues(self, issues, revision, mercurial_repository=None, bulk=0):
|
||||
def publish_issues(self, issues, revision, bulk=0):
|
||||
"""
|
||||
Publish all issues on the backend.
|
||||
`mercurial_repository` parameter should be path to the local repository at the given revision.
|
||||
If set to an integer, `bulk` parameter allow to use the bulk creation endpoint of the backend.
|
||||
"""
|
||||
if not self.enabled:
|
||||
|
|
@ -133,14 +132,13 @@ class BackendAPI(object):
|
|||
valid_data = []
|
||||
# Build issues' payload for that given chunk
|
||||
for issue in issues_chunk:
|
||||
issue_hash = issue.build_hash(local_repository=mercurial_repository)
|
||||
if issue_hash is None:
|
||||
if issue.hash is None:
|
||||
logger.warning(
|
||||
"Missing issue hash, cannot publish on backend",
|
||||
issue=str(issue),
|
||||
)
|
||||
continue
|
||||
valid_data.append((issue, issue.as_dict(issue_hash=issue_hash)))
|
||||
valid_data.append((issue, issue.as_dict()))
|
||||
|
||||
response = self.create(
|
||||
revision.issues_url,
|
||||
|
|
@ -162,16 +160,15 @@ class BackendAPI(object):
|
|||
|
||||
for issue in issues:
|
||||
try:
|
||||
issue_hash = issue.build_hash(local_repository=mercurial_repository)
|
||||
assert issue.hash is not None
|
||||
except Exception:
|
||||
issue_hash = None
|
||||
if issue_hash is None:
|
||||
# An exception may occur computing issues hash (e.g. network error)
|
||||
logger.warning(
|
||||
"Missing issue hash, cannot publish on backend",
|
||||
issue=str(issue),
|
||||
)
|
||||
continue
|
||||
payload = issue.as_dict(issue_hash=issue_hash)
|
||||
payload = issue.as_dict()
|
||||
issue.on_backend = self.create(revision.diff_issues_url, payload)
|
||||
if issue.on_backend is not None:
|
||||
published += 1
|
||||
|
|
@ -238,3 +235,24 @@ class BackendAPI(object):
|
|||
out = response.json()
|
||||
logger.info("Created item on backend", url=url_post, id=out.get("id"))
|
||||
return out
|
||||
|
||||
def list_repo_issues(
|
||||
self, repo_slug, date=None, revision_changeset=None, path=None
|
||||
):
|
||||
"""
|
||||
List issues detected from a specific repository.
|
||||
Optional `date` and `revision_id` parameters can be used to look for a
|
||||
specific revision (defaults to the revision closest to the given date).
|
||||
"""
|
||||
params = {
|
||||
key: value
|
||||
for key, value in (
|
||||
("path", path),
|
||||
("date", date),
|
||||
("revision_changeset", revision_changeset),
|
||||
)
|
||||
if value is not None
|
||||
}
|
||||
return list(
|
||||
self.paginate(f"/v1/issues/{repo_slug}/?{urllib.parse.urlencode(params)}")
|
||||
)
|
||||
|
|
|
|||
|
|
@ -104,6 +104,13 @@ def main():
|
|||
taskcluster.secrets["ALLOWED_PATHS"],
|
||||
taskcluster.secrets["repositories"],
|
||||
)
|
||||
|
||||
# Store some CLI parameters in the global settings
|
||||
settings.runtime.update(
|
||||
{
|
||||
"mercurial_repository": args.mercurial_repository,
|
||||
}
|
||||
)
|
||||
# Setup statistics
|
||||
influx_conf = taskcluster.secrets.get("influxdb")
|
||||
if influx_conf:
|
||||
|
|
@ -182,7 +189,6 @@ def main():
|
|||
# Update build status only when phabricator reporting is enabled
|
||||
update_build=phabricator_reporting_enabled,
|
||||
task_failures_ignored=taskcluster.secrets["task_failures_ignored"],
|
||||
mercurial_repository=args.mercurial_repository,
|
||||
)
|
||||
try:
|
||||
if settings.autoland_group_id:
|
||||
|
|
|
|||
|
|
@ -11,6 +11,7 @@ import fnmatch
|
|||
import os
|
||||
import shutil
|
||||
import tempfile
|
||||
from contextlib import contextmanager
|
||||
|
||||
import pkg_resources
|
||||
import structlog
|
||||
|
|
@ -51,6 +52,8 @@ class Settings(object):
|
|||
self.hgmo_cache = tempfile.mkdtemp(suffix="hgmo")
|
||||
self.repositories = []
|
||||
self.decision_env_prefixes = []
|
||||
# Runtime settings
|
||||
self.runtime = {}
|
||||
|
||||
# Always cleanup at the end of the execution
|
||||
atexit.register(self.cleanup)
|
||||
|
|
@ -127,6 +130,16 @@ class Settings(object):
|
|||
"""
|
||||
return any([fnmatch.fnmatch(path, rule) for rule in self.allowed_paths])
|
||||
|
||||
@contextmanager
|
||||
def override_runtime_setting(self, key, value):
|
||||
"""
|
||||
Overrides a runtime setting, then restores the default value
|
||||
"""
|
||||
copy = {**self.runtime}
|
||||
self.runtime[key] = value
|
||||
yield
|
||||
self.runtime = copy
|
||||
|
||||
def cleanup(self):
|
||||
shutil.rmtree(self.hgmo_cache)
|
||||
|
||||
|
|
|
|||
|
|
@ -10,7 +10,7 @@ from urllib.parse import urljoin
|
|||
import structlog
|
||||
from libmozdata.phabricator import BuildState, PhabricatorAPI
|
||||
|
||||
from code_review_bot import Issue, stats
|
||||
from code_review_bot import Issue, Level, stats
|
||||
from code_review_bot.backend import BackendAPI
|
||||
from code_review_bot.report.base import Reporter
|
||||
from code_review_bot.tasks.clang_tidy_external import ExternalTidyIssue
|
||||
|
|
@ -20,7 +20,7 @@ from code_review_tools import treeherder
|
|||
BUG_REPORT_URL = "https://bugzilla.mozilla.org/enter_bug.cgi?product=Developer+Infrastructure&component=Source+Code+Analysis&short_desc=[Automated+review]+THIS+IS+A+PLACEHOLDER&comment=**Phabricator+URL:**+https://phabricator.services.mozilla.com/...&format=__default__"
|
||||
|
||||
COMMENT_FAILURE = """
|
||||
Code analysis found {defects_total}{defects_details} in the diff [{diff_id}]({phabricator_diff_url}):
|
||||
Code analysis found {defects_total} in diff [{diff_id}]({phabricator_diff_url}):
|
||||
"""
|
||||
|
||||
COMMENT_WARNINGS = """
|
||||
|
|
@ -49,8 +49,6 @@ Should they have tests, or are they dead code?
|
|||
|
||||
# Use two line breaks to force '---' to be rendered as a horizontal rule in Phabricator's markdown
|
||||
BUG_REPORT = """
|
||||
|
||||
---
|
||||
If you see a problem in this automated review, [please report it here]({bug_report_url}).
|
||||
"""
|
||||
|
||||
|
|
@ -59,7 +57,7 @@ For your convenience, [here is a patch]({url}) that fixes all the {analyzer} def
|
|||
"""
|
||||
|
||||
COMMENT_TASK_FAILURE = """
|
||||
The analysis task [{name}]({url}) failed, but we could not detect any issue.
|
||||
The analysis task [{name}]({url}) failed, but we could not detect any defect.
|
||||
Please check this task manually.
|
||||
"""
|
||||
|
||||
|
|
@ -104,6 +102,15 @@ class PhabricatorReporter(Reporter):
|
|||
self.api = api
|
||||
logger.info("Phabricator reporter enabled")
|
||||
|
||||
@staticmethod
|
||||
def pluralize(word, nb):
|
||||
"""
|
||||
Simple helper to pluralize a noun depending of the number of elements
|
||||
"""
|
||||
assert isinstance(word, str)
|
||||
assert isinstance(nb, int)
|
||||
return "{} {}".format(nb, nb == 1 and word or word + "s")
|
||||
|
||||
def compare_issues(self, former_diff_id, issues):
|
||||
"""
|
||||
Compare new issues depending on their evolution from the
|
||||
|
|
@ -179,7 +186,7 @@ class PhabricatorReporter(Reporter):
|
|||
|
||||
# Use only new and publishable issues and patches
|
||||
# Avoid publishing a patch from a de-activated analyzer
|
||||
issues = [
|
||||
publishable_issues = [
|
||||
issue
|
||||
for issue in issues
|
||||
if issue.is_publishable()
|
||||
|
|
@ -191,9 +198,9 @@ class PhabricatorReporter(Reporter):
|
|||
if patch.analyzer.name not in self.analyzers_skipped
|
||||
]
|
||||
|
||||
if issues:
|
||||
if publishable_issues:
|
||||
# Publish detected patch's issues on Harbormaster, all at once, as lint issues
|
||||
self.publish_harbormaster(revision, issues)
|
||||
self.publish_harbormaster(revision, publishable_issues)
|
||||
|
||||
# Retrieve all diffs for the current revision
|
||||
rev_diffs = self.api.search_diffs(revision_phid=revision.phabricator_phid)
|
||||
|
|
@ -202,16 +209,19 @@ class PhabricatorReporter(Reporter):
|
|||
logger.warning(
|
||||
"A newer diff exists on this patch, skipping the comment publication"
|
||||
)
|
||||
return issues, patches
|
||||
return publishable_issues, patches
|
||||
|
||||
# Compare issues that are not known on the repository to a previous diff
|
||||
older_diff_ids = [
|
||||
diff["id"] for diff in rev_diffs if diff["id"] < revision.diff_id
|
||||
]
|
||||
former_diff_id = sorted(older_diff_ids)[-1] if older_diff_ids else None
|
||||
unresolved_issues, closed_issues = self.compare_issues(former_diff_id, issues)
|
||||
unresolved_issues, closed_issues = self.compare_issues(
|
||||
former_diff_id, publishable_issues
|
||||
)
|
||||
|
||||
if (
|
||||
len(unresolved_issues) == len(issues)
|
||||
len(unresolved_issues) == len(publishable_issues)
|
||||
and not closed_issues
|
||||
and not task_failures
|
||||
and not notices
|
||||
|
|
@ -222,12 +232,12 @@ class PhabricatorReporter(Reporter):
|
|||
"Skipping comment publication (some issues are unresolved)",
|
||||
unresolved_count=len(unresolved_issues),
|
||||
)
|
||||
return issues, patches
|
||||
return publishable_issues, patches
|
||||
|
||||
# Publish comment summarizing detected, unresolved and closed issues
|
||||
self.publish_summary(
|
||||
revision,
|
||||
issues,
|
||||
publishable_issues,
|
||||
patches,
|
||||
task_failures,
|
||||
notices,
|
||||
|
|
@ -240,7 +250,7 @@ class PhabricatorReporter(Reporter):
|
|||
stats.add_metric("report.phabricator.issues", len(issues))
|
||||
stats.add_metric("report.phabricator")
|
||||
|
||||
return issues, patches
|
||||
return publishable_issues, patches
|
||||
|
||||
def publish_harbormaster(
|
||||
self, revision, lint_issues: Issues = [], unit_issues: Issues = []
|
||||
|
|
@ -308,44 +318,7 @@ class PhabricatorReporter(Reporter):
|
|||
"""
|
||||
Build a Markdown comment about published issues
|
||||
"""
|
||||
|
||||
def pluralize(word, nb):
|
||||
assert isinstance(word, str)
|
||||
assert isinstance(nb, int)
|
||||
return "{} {}".format(nb, nb == 1 and word or word + "s")
|
||||
|
||||
# List all the issues classes
|
||||
issue_classes = {issue.__class__ for issue in issues}
|
||||
|
||||
# Calc stats for issues, grouped by class
|
||||
stats = self.calc_stats(issues)
|
||||
|
||||
# Build parts depending on issues
|
||||
defects, analyzers = set(), set()
|
||||
total_warnings = 0
|
||||
total_errors = 0
|
||||
for stat in stats:
|
||||
defect_nb = []
|
||||
if stat["nb_build_errors"] > 0:
|
||||
defect_nb.append(pluralize("build error", stat["nb_build_errors"]))
|
||||
if stat["nb_defects"] > 0:
|
||||
defect_nb.append(pluralize("defect", stat["nb_defects"]))
|
||||
|
||||
defects.add(
|
||||
" - {nb} found by {analyzer}".format(
|
||||
analyzer=stat["analyzer"], nb=" and ".join(defect_nb)
|
||||
)
|
||||
)
|
||||
_help = stat.get("help")
|
||||
if _help is not None:
|
||||
analyzers.add(f" - {_help}")
|
||||
|
||||
total_warnings += stat["nb_warnings"]
|
||||
total_errors += stat["nb_errors"]
|
||||
|
||||
# Order both sets
|
||||
defects = sorted(defects)
|
||||
analyzers = sorted(analyzers)
|
||||
comment = ""
|
||||
|
||||
# Comment with an absolute link to the revision diff in Phabricator
|
||||
# Relative links are not supported in comment and non readable in related emails
|
||||
|
|
@ -353,34 +326,29 @@ class PhabricatorReporter(Reporter):
|
|||
diff_id=revision.diff_id
|
||||
)
|
||||
|
||||
# Build top comment
|
||||
nb = len(issues)
|
||||
|
||||
# Add extra hint when errors are published outside of the patch
|
||||
defects_details = ""
|
||||
# Only display the hint when the revision has parents in his stack
|
||||
rev_parents_count = len(self.api.load_parents(revision.phabricator_phid))
|
||||
external_failures_count = rev_parents_count and sum(
|
||||
1
|
||||
for issue in issues
|
||||
if issue.is_publishable() and not revision.contains(issue)
|
||||
)
|
||||
if nb == 1 and external_failures_count == 1:
|
||||
defects_details = " (in a parent revision)"
|
||||
elif nb > 1 and external_failures_count > 0:
|
||||
defects_details = f" ({external_failures_count} in a parent revision)"
|
||||
|
||||
if nb > 0:
|
||||
# Build main comment for issues inside the patch
|
||||
if (nb := len(issues)) > 0:
|
||||
comment = COMMENT_FAILURE.format(
|
||||
defects_total=pluralize("defect", nb),
|
||||
defects_details=defects_details,
|
||||
defects_total=self.pluralize("defect", nb),
|
||||
diff_id=revision.diff_id,
|
||||
phabricator_diff_url=phabricator_diff_url,
|
||||
)
|
||||
else:
|
||||
comment = ""
|
||||
|
||||
# Add defects
|
||||
# Calc stats for issues inside the patch, grouped by class and sorted by number of issues
|
||||
defects = []
|
||||
stats = self.calc_stats(issues)
|
||||
for stat in sorted(stats, key=lambda x: (x["total"], x["analyzer"])):
|
||||
defect_nb = []
|
||||
if stat["nb_build_errors"] > 0:
|
||||
defect_nb.append(self.pluralize("build error", stat["nb_build_errors"]))
|
||||
if stat["nb_defects"] > 0:
|
||||
defect_nb.append(self.pluralize("defect", stat["nb_defects"]))
|
||||
|
||||
defects.append(
|
||||
" - {nb} found by {analyzer}".format(
|
||||
analyzer=stat["analyzer"], nb=" and ".join(defect_nb)
|
||||
)
|
||||
)
|
||||
if defects:
|
||||
comment += "\n".join(defects) + "\n"
|
||||
|
||||
|
|
@ -388,11 +356,13 @@ class PhabricatorReporter(Reporter):
|
|||
if unresolved or closed:
|
||||
followup_comment = ""
|
||||
if unresolved:
|
||||
followup_comment += pluralize("issue", unresolved) + " unresolved "
|
||||
followup_comment += (
|
||||
self.pluralize("defect", unresolved) + " unresolved "
|
||||
)
|
||||
if closed:
|
||||
followup_comment += "and "
|
||||
if closed:
|
||||
followup_comment += pluralize("issue", closed) + " closed "
|
||||
followup_comment += self.pluralize("defect", closed) + " closed "
|
||||
followup_comment += COMMENT_DIFF_FOLLOWUP.format(
|
||||
phabricator_diff_url=self.phabricator_diff_url.format(
|
||||
diff_id=former_diff_id
|
||||
|
|
@ -402,15 +372,25 @@ class PhabricatorReporter(Reporter):
|
|||
comment += followup_comment
|
||||
|
||||
# Add colored warning section
|
||||
total_warnings = sum(1 for i in issues if i.level == Level.Warning)
|
||||
if total_warnings:
|
||||
comment += COMMENT_WARNINGS.format(
|
||||
nb_warnings=pluralize("issue", total_warnings)
|
||||
nb_warnings=self.pluralize("defect", total_warnings)
|
||||
)
|
||||
|
||||
# Add colored error section
|
||||
total_errors = sum(1 for i in issues if i.level == Level.Error)
|
||||
if total_errors:
|
||||
comment += COMMENT_ERRORS.format(nb_errors=pluralize("issue", total_errors))
|
||||
comment += COMMENT_ERRORS.format(
|
||||
nb_errors=self.pluralize("defect", total_errors)
|
||||
)
|
||||
|
||||
# Build analyzers help comment for all issues
|
||||
analyzers = set()
|
||||
for stat in stats:
|
||||
_help = stat.get("help")
|
||||
if _help is not None:
|
||||
analyzers.add(f" - {_help}")
|
||||
if analyzers:
|
||||
comment += COMMENT_RUN_ANALYZERS.format(analyzers="\n".join(analyzers))
|
||||
|
||||
|
|
@ -434,6 +414,7 @@ class PhabricatorReporter(Reporter):
|
|||
comment += COMMENT_TASK_FAILURE.format(name=task.name, url=treeherder_url)
|
||||
|
||||
# Add coverage reporting details when a coverage issue is published
|
||||
issue_classes = {issue.__class__ for issue in issues}
|
||||
if CoverageIssue in issue_classes:
|
||||
comment += COMMENT_COVERAGE
|
||||
|
||||
|
|
@ -443,6 +424,9 @@ class PhabricatorReporter(Reporter):
|
|||
|
||||
assert comment != "", "Empty comment"
|
||||
|
||||
# Display more information in the footer section
|
||||
comment += "\n\n---\n"
|
||||
|
||||
comment += BUG_REPORT.format(bug_report_url=bug_report_url)
|
||||
|
||||
if defects:
|
||||
|
|
@ -450,4 +434,5 @@ class PhabricatorReporter(Reporter):
|
|||
diff_id=revision.diff_id,
|
||||
phabricator_diff_url=phabricator_diff_url,
|
||||
)
|
||||
|
||||
return comment
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@
|
|||
# file, You can obtain one at http://mozilla.org/MPL/2.0/.
|
||||
|
||||
import os
|
||||
import random
|
||||
import urllib.parse
|
||||
from datetime import timedelta
|
||||
from pathlib import Path
|
||||
|
|
@ -138,6 +139,24 @@ class Revision(object):
|
|||
"phabricator.diffphid.{}".format(self.diff_phid),
|
||||
]
|
||||
|
||||
@property
|
||||
def before_after_feature(self):
|
||||
"""
|
||||
Randomly run the before/after feature depending on a configured ratio.
|
||||
All the diffs of a revision must be analysed with or without the feature.
|
||||
"""
|
||||
if getattr(self, "id", None) is None:
|
||||
logger.debug(
|
||||
"Backend ID must be set to determine if using the before/after feature. Skipping."
|
||||
)
|
||||
return False
|
||||
# Set random module pseudo-random seed based on the revision ID to
|
||||
# ensure that successive calls to random.random will return deterministic values
|
||||
random.seed(self.id)
|
||||
return random.random() < taskcluster.secrets.get("BEFORE_AFTER_RATIO", 0)
|
||||
# Reset random module seed to prevent deterministic values after calling that function
|
||||
random.seed(os.urandom(128))
|
||||
|
||||
def __repr__(self):
|
||||
return self.diff_phid
|
||||
|
||||
|
|
|
|||
|
|
@ -5,6 +5,7 @@
|
|||
|
||||
from contextlib import nullcontext
|
||||
from datetime import datetime, timedelta
|
||||
from itertools import groupby
|
||||
|
||||
import structlog
|
||||
from libmozdata.phabricator import BuildState, PhabricatorAPI
|
||||
|
|
@ -52,7 +53,6 @@ class Workflow(object):
|
|||
zero_coverage_enabled=True,
|
||||
update_build=True,
|
||||
task_failures_ignored=[],
|
||||
mercurial_repository=None,
|
||||
):
|
||||
self.zero_coverage_enabled = zero_coverage_enabled
|
||||
self.update_build = update_build
|
||||
|
|
@ -80,9 +80,6 @@ class Workflow(object):
|
|||
# Setup Backend API client
|
||||
self.backend_api = BackendAPI()
|
||||
|
||||
# Path to the mercurial repository
|
||||
self.mercurial_repository = mercurial_repository
|
||||
|
||||
def run(self, revision):
|
||||
"""
|
||||
Find all issues on remote tasks and publish them
|
||||
|
|
@ -100,8 +97,36 @@ class Workflow(object):
|
|||
issues, task_failures, notices, reviewers = self.find_issues(
|
||||
revision, settings.try_group_id
|
||||
)
|
||||
if not issues and not task_failures and not notices:
|
||||
logger.info("No issues or notices, stopping there.")
|
||||
|
||||
# Analyze issues in case the before/after feature is enabled
|
||||
if revision.before_after_feature:
|
||||
logger.info("Running the before/after feature")
|
||||
# Search a base revision from the decision task
|
||||
decision = self.queue_service.task(settings.try_group_id)
|
||||
base_rev_changeset = (
|
||||
decision.get("payload", {}).get("env", {}).get("GECKO_BASE_REV")
|
||||
)
|
||||
if not base_rev_changeset:
|
||||
logger.warning(
|
||||
"Base revision changeset could not be fetched from Phabricator, "
|
||||
"looking for existing issues based on the current date",
|
||||
task=settings.try_group_id,
|
||||
)
|
||||
|
||||
# Mark know issues to avoid publishing them on this patch
|
||||
self.find_previous_issues(issues, base_rev_changeset)
|
||||
new_issues_count = sum(issue.new_issue for issue in issues)
|
||||
logger.info(
|
||||
f"Found {new_issues_count} new issues (over {len(issues)} total detected issues)",
|
||||
task=settings.try_group_id,
|
||||
)
|
||||
|
||||
if (
|
||||
all(issue.new_issue is False for issue in issues)
|
||||
and not task_failures
|
||||
and not notices
|
||||
):
|
||||
logger.info("No issues nor notices, stopping there.")
|
||||
|
||||
# Publish all issues
|
||||
self.publish(revision, issues, task_failures, notices, reviewers)
|
||||
|
|
@ -178,26 +203,28 @@ class Workflow(object):
|
|||
logger.info("No issues for that revision")
|
||||
return
|
||||
|
||||
context_manager = nullcontext(self.mercurial_repository)
|
||||
runtime_repo = settings.runtime.get("mercurial_repository")
|
||||
context_manager = nullcontext(runtime_repo)
|
||||
# Do always clone the repository on production to speed up reading issues
|
||||
if (
|
||||
self.mercurial_repository is None
|
||||
and settings.taskcluster.task_id != "local instance"
|
||||
):
|
||||
if runtime_repo is None and settings.taskcluster.task_id != "local instance":
|
||||
logger.info(
|
||||
f"Cloning revision to build issues (checkout to {revision.head_changeset})"
|
||||
"Cloning revision to build issues",
|
||||
repo=revision.head_repository,
|
||||
changeset=revision.head_changeset,
|
||||
)
|
||||
context_manager = clone_repository(
|
||||
repo_url=revision.head_repository, branch=revision.head_changeset
|
||||
)
|
||||
|
||||
with context_manager as repo_path:
|
||||
self.backend_api.publish_issues(
|
||||
issues,
|
||||
revision,
|
||||
mercurial_repository=repo_path,
|
||||
bulk=BULK_ISSUE_CHUNKS,
|
||||
)
|
||||
# Override runtime settings with the cloned repository
|
||||
with settings.override_runtime_setting("mercurial_repository", repo_path):
|
||||
# Publish issues in the backend
|
||||
self.backend_api.publish_issues(
|
||||
issues,
|
||||
revision,
|
||||
bulk=BULK_ISSUE_CHUNKS,
|
||||
)
|
||||
|
||||
def publish(self, revision, issues, task_failures, notices, reviewers):
|
||||
"""
|
||||
|
|
@ -212,7 +239,6 @@ class Workflow(object):
|
|||
patch.publish()
|
||||
|
||||
# Publish issues on backend to retrieve their comparison state
|
||||
# Only publish errors and "in patch" warnings due to a backend timeout
|
||||
publishable_issues = [i for i in issues if i.is_publishable()]
|
||||
|
||||
self.backend_api.publish_issues(publishable_issues, revision)
|
||||
|
|
@ -306,6 +332,40 @@ class Workflow(object):
|
|||
},
|
||||
)
|
||||
|
||||
def find_previous_issues(self, issues, base_rev_changeset=None):
|
||||
"""
|
||||
Look for known issues in the backend matching the given list of issues
|
||||
|
||||
If a base revision ID is provided, compare to issues detected on this revision
|
||||
Otherwise, compare to issues detected on last ingested revision
|
||||
"""
|
||||
assert (
|
||||
self.backend_api.enabled
|
||||
), "Backend storage is disabled, comparing issues is not possible"
|
||||
|
||||
current_date = datetime.now().strftime("%Y-%m-%d")
|
||||
|
||||
# Group issues by path, so we only list know issues for the affected files
|
||||
issues_groups = groupby(
|
||||
sorted(issues, key=lambda i: i.path),
|
||||
lambda i: i.path,
|
||||
)
|
||||
logger.info(
|
||||
"Checking for existing issues in the backend",
|
||||
base_revision_changeset=base_rev_changeset,
|
||||
)
|
||||
|
||||
for path, group_issues in issues_groups:
|
||||
known_issues = self.backend_api.list_repo_issues(
|
||||
"mozilla-central",
|
||||
date=current_date,
|
||||
revision_changeset=base_rev_changeset,
|
||||
path=path,
|
||||
)
|
||||
hashes = [issue["hash"] for issue in known_issues]
|
||||
for issue in group_issues:
|
||||
issue.new_issue = bool(issue.hash and issue.hash not in hashes)
|
||||
|
||||
def find_issues(self, revision, group_id):
|
||||
"""
|
||||
Find all issues on remote Taskcluster task group
|
||||
|
|
|
|||
|
|
@ -97,7 +97,7 @@ A random issue happened here
|
|||
"""
|
||||
)
|
||||
|
||||
assert issue.build_hash() == "533d1aefc79ef542b3e7d677c1c5724e"
|
||||
assert issue.hash == "533d1aefc79ef542b3e7d677c1c5724e"
|
||||
assert issue.as_dict() == {
|
||||
"analyzer": "any-analyzer-name",
|
||||
"check": "XYZ",
|
||||
|
|
|
|||
|
|
@ -10,7 +10,7 @@ import pytest
|
|||
from code_review_bot.tasks.lint import MozLintIssue, MozLintTask
|
||||
|
||||
|
||||
def test_build_hash(mock_revision, mock_hgmo, mock_task):
|
||||
def test_get_hash(mock_revision, mock_hgmo, mock_task):
|
||||
"""
|
||||
Test build hash algorithm
|
||||
"""
|
||||
|
|
@ -51,7 +51,7 @@ def test_build_hash(mock_revision, mock_hgmo, mock_task):
|
|||
"A random & fake linting issue"
|
||||
)
|
||||
hash_check = hashlib.md5(payload.encode("utf-8")).hexdigest()
|
||||
assert hash_check == "b06e5b92a609496d1473ca90fec1749c" == issue.build_hash()
|
||||
assert hash_check == "b06e5b92a609496d1473ca90fec1749c" == issue.hash
|
||||
|
||||
|
||||
def test_indentation_effect(mock_revision, mock_hgmo, mock_task):
|
||||
|
|
@ -94,9 +94,7 @@ def test_indentation_effect(mock_revision, mock_hgmo, mock_task):
|
|||
|
||||
# Check the hashes are equal
|
||||
assert (
|
||||
issue_indent.build_hash()
|
||||
== issue_no_indent.build_hash()
|
||||
== "a8c5c52b21c12b483617adc60cdd5dc2"
|
||||
issue_indent.hash == issue_no_indent.hash == "a8c5c52b21c12b483617adc60cdd5dc2"
|
||||
)
|
||||
|
||||
|
||||
|
|
@ -126,7 +124,7 @@ def test_full_file(mock_revision, mock_hgmo, mock_task):
|
|||
assert issue.line is None
|
||||
|
||||
# Build hash should use the full file
|
||||
assert issue.build_hash() == "65fe9040e64b3617e4cbf40ef478f62d"
|
||||
assert issue.hash == "65fe9040e64b3617e4cbf40ef478f62d"
|
||||
|
||||
# Check positive integers or None are used in report
|
||||
assert issue.as_dict() == {
|
||||
|
|
|
|||
|
|
@ -140,4 +140,4 @@ def test_licence_payload(mock_revision, mock_hgmo):
|
|||
== "source-test-mozlint-license issue source-test-mozlint-license@error intl/locale/rust/unic-langid-ffi/src/lib.rs full file"
|
||||
)
|
||||
assert issue.check == issue.analyzer.name == "source-test-mozlint-license"
|
||||
assert issue.build_hash() == "7142c536e10b31925b018c37b0e6f9f8"
|
||||
assert issue.hash == "7142c536e10b31925b018c37b0e6f9f8"
|
||||
|
|
|
|||
|
|
@ -26,42 +26,44 @@ from code_review_bot.tasks.lint import MozLintIssue, MozLintTask
|
|||
from code_review_bot.tasks.tgdiff import COMMENT_TASKGRAPH_DIFF
|
||||
|
||||
VALID_CLANG_TIDY_MESSAGE = """
|
||||
Code analysis found 1 defect in the diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
Code analysis found 1 defect in diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
- 1 defect found by clang-tidy
|
||||
|
||||
WARNING: Found 1 issue (warning level) that can be dismissed.
|
||||
WARNING: Found 1 defect (warning level) that can be dismissed.
|
||||
|
||||
You can run this analysis locally with:
|
||||
- `./mach static-analysis check --outgoing` (C/C++)
|
||||
|
||||
|
||||
---
|
||||
|
||||
If you see a problem in this automated review, [please report it here](https://bugzilla.mozilla.org/enter_bug.cgi?product=Developer+Infrastructure&component=Source+Code+Analysis&short_desc=[Automated+review]+THIS+IS+A+PLACEHOLDER&comment=**Phabricator+URL:**+https://phabricator.services.mozilla.com/...&format=__default__).
|
||||
|
||||
You can view these defects in the Diff Detail section of [Phabricator diff 42](https://phabricator.services.mozilla.com/differential/diff/42/).
|
||||
"""
|
||||
|
||||
VALID_BUILD_ERROR_MESSAGE = """
|
||||
Code analysis found 1 defect (in a parent revision) in the diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
Code analysis found 1 defect in diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
- 1 build error found by clang-tidy
|
||||
|
||||
IMPORTANT: Found 1 issue (error level) that must be fixed before landing.
|
||||
IMPORTANT: Found 1 defect (error level) that must be fixed before landing.
|
||||
|
||||
You can run this analysis locally with:
|
||||
- `./mach static-analysis check --outgoing` (C/C++)
|
||||
|
||||
|
||||
---
|
||||
|
||||
If you see a problem in this automated review, [please report it here](https://bugzilla.mozilla.org/enter_bug.cgi?product=Developer+Infrastructure&component=Source+Code+Analysis&short_desc=[Automated+review]+THIS+IS+A+PLACEHOLDER&comment=**Phabricator+URL:**+https://phabricator.services.mozilla.com/...&format=__default__).
|
||||
|
||||
You can view these defects in the Diff Detail section of [Phabricator diff 42](https://phabricator.services.mozilla.com/differential/diff/42/).
|
||||
"""
|
||||
|
||||
VALID_CLANG_FORMAT_MESSAGE = """
|
||||
Code analysis found 1 defect in the diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
Code analysis found 1 defect in diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
- 1 defect found by clang-format
|
||||
|
||||
WARNING: Found 1 issue (warning level) that can be dismissed.
|
||||
WARNING: Found 1 defect (warning level) that can be dismissed.
|
||||
|
||||
You can run this analysis locally with:
|
||||
- `./mach clang-format -p dom/test.cpp`
|
||||
|
|
@ -70,35 +72,37 @@ For your convenience, [here is a patch]({results}/source-test-clang-format-PHID-
|
|||
|
||||
|
||||
---
|
||||
|
||||
If you see a problem in this automated review, [please report it here](https://bugzilla.mozilla.org/enter_bug.cgi?product=Developer+Infrastructure&component=Source+Code+Analysis&short_desc=[Automated+review]+THIS+IS+A+PLACEHOLDER&comment=**Phabricator+URL:**+https://phabricator.services.mozilla.com/...&format=__default__).
|
||||
|
||||
You can view these defects in the Diff Detail section of [Phabricator diff 42](https://phabricator.services.mozilla.com/differential/diff/42/).
|
||||
"""
|
||||
|
||||
VALID_FLAKE8_MESSAGE = """
|
||||
Code analysis found 2 defects in the diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
Code analysis found 2 defects in diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
- 1 defect found by eslint (Mozlint)
|
||||
- 1 defect found by py-flake8 (Mozlint)
|
||||
|
||||
WARNING: Found 1 issue (warning level) that can be dismissed.
|
||||
WARNING: Found 1 defect (warning level) that can be dismissed.
|
||||
|
||||
IMPORTANT: Found 1 issue (error level) that must be fixed before landing.
|
||||
IMPORTANT: Found 1 defect (error level) that must be fixed before landing.
|
||||
|
||||
You can run this analysis locally with:
|
||||
- `./mach lint --warnings --outgoing`
|
||||
|
||||
|
||||
---
|
||||
|
||||
If you see a problem in this automated review, [please report it here](https://bugzilla.mozilla.org/enter_bug.cgi?product=Developer+Infrastructure&component=Source+Code+Analysis&short_desc=[Automated+review]+THIS+IS+A+PLACEHOLDER&comment=**Phabricator+URL:**+https://phabricator.services.mozilla.com/...&format=__default__).
|
||||
|
||||
You can view these defects in the Diff Detail section of [Phabricator diff 42](https://phabricator.services.mozilla.com/differential/diff/42/).
|
||||
"""
|
||||
|
||||
VALID_COVERAGE_MESSAGE = """
|
||||
Code analysis found 1 defect in the diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
Code analysis found 1 defect in diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
- 1 defect found by code coverage analysis
|
||||
|
||||
WARNING: Found 1 issue (warning level) that can be dismissed.
|
||||
WARNING: Found 1 defect (warning level) that can be dismissed.
|
||||
|
||||
In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
|
||||
Should they have tests, or are they dead code?
|
||||
|
|
@ -108,57 +112,61 @@ Should they have tests, or are they dead code?
|
|||
|
||||
|
||||
---
|
||||
|
||||
If you see a problem in this automated review, [please report it here](https://bugzilla.mozilla.org/enter_bug.cgi?product=Developer+Infrastructure&component=Source+Code+Analysis&short_desc=[Automated+review]+THIS+IS+A+PLACEHOLDER&comment=**Phabricator+URL:**+https://phabricator.services.mozilla.com/...&format=__default__).
|
||||
|
||||
You can view these defects in the Diff Detail section of [Phabricator diff 42](https://phabricator.services.mozilla.com/differential/diff/42/).
|
||||
"""
|
||||
|
||||
VALID_DEFAULT_MESSAGE = """
|
||||
Code analysis found 1 defect in the diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
Code analysis found 1 defect in diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
- 1 defect found by full-file-analyzer
|
||||
|
||||
WARNING: Found 1 issue (warning level) that can be dismissed.
|
||||
WARNING: Found 1 defect (warning level) that can be dismissed.
|
||||
|
||||
|
||||
---
|
||||
|
||||
If you see a problem in this automated review, [please report it here](https://bugzilla.mozilla.org/enter_bug.cgi?product=Developer+Infrastructure&component=Source+Code+Analysis&short_desc=[Automated+review]+THIS+IS+A+PLACEHOLDER&comment=**Phabricator+URL:**+https://phabricator.services.mozilla.com/...&format=__default__).
|
||||
|
||||
You can view these defects in the Diff Detail section of [Phabricator diff 42](https://phabricator.services.mozilla.com/differential/diff/42/).
|
||||
"""
|
||||
|
||||
VALID_TASK_FAILURES_MESSAGE = """
|
||||
The analysis task [mock-clang-tidy](https://treeherder.mozilla.org/#/jobs?repo=try&revision=deadc0ffee&selectedTaskRun=ab3NrysvSZyEwsOHL2MZfw-0) failed, but we could not detect any issue.
|
||||
The analysis task [mock-clang-tidy](https://treeherder.mozilla.org/#/jobs?repo=try&revision=deadc0ffee&selectedTaskRun=ab3NrysvSZyEwsOHL2MZfw-0) failed, but we could not detect any defect.
|
||||
Please check this task manually.
|
||||
|
||||
|
||||
---
|
||||
|
||||
If you see a problem in this automated review, [please report it here](https://bugzilla.mozilla.org/enter_bug.cgi?product=Developer+Infrastructure&component=Source+Code+Analysis&short_desc=[Automated+review]+THIS+IS+A+PLACEHOLDER&comment=**Phabricator+URL:**+https://phabricator.services.mozilla.com/...&format=__default__).
|
||||
"""
|
||||
|
||||
VALID_MOZLINT_MESSAGE = """
|
||||
Code analysis found 2 defects (1 in a parent revision) in the diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
Code analysis found 2 defects in diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
- 2 defects found by dummy (Mozlint)
|
||||
|
||||
WARNING: Found 1 issue (warning level) that can be dismissed.
|
||||
WARNING: Found 1 defect (warning level) that can be dismissed.
|
||||
|
||||
IMPORTANT: Found 1 issue (error level) that must be fixed before landing.
|
||||
IMPORTANT: Found 1 defect (error level) that must be fixed before landing.
|
||||
|
||||
You can run this analysis locally with:
|
||||
- `./mach lint --warnings --outgoing`
|
||||
|
||||
|
||||
---
|
||||
|
||||
If you see a problem in this automated review, [please report it here](https://bugzilla.mozilla.org/enter_bug.cgi?product=Developer+Infrastructure&component=Source+Code+Analysis&short_desc=[Automated+review]+THIS+IS+A+PLACEHOLDER&comment=**Phabricator+URL:**+https://phabricator.services.mozilla.com/...&format=__default__).
|
||||
|
||||
You can view these defects in the Diff Detail section of [Phabricator diff 42](https://phabricator.services.mozilla.com/differential/diff/42/).
|
||||
"""
|
||||
|
||||
VALID_CLANG_TIDY_COVERAGE_MESSAGE = """
|
||||
Code analysis found 2 defects in the diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
Code analysis found 2 defects in diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
- 1 defect found by clang-tidy
|
||||
- 1 defect found by code coverage analysis
|
||||
|
||||
WARNING: Found 2 issues (warning level) that can be dismissed.
|
||||
WARNING: Found 2 defects (warning level) that can be dismissed.
|
||||
|
||||
You can run this analysis locally with:
|
||||
- `./mach static-analysis check --outgoing` (C/C++)
|
||||
|
|
@ -171,6 +179,7 @@ Should they have tests, or are they dead code?
|
|||
|
||||
|
||||
---
|
||||
|
||||
If you see a problem in this automated review, [please report it here](https://bugzilla.mozilla.org/enter_bug.cgi?product=Developer+Infrastructure&component=Source+Code+Analysis&short_desc=[Automated+review]+THIS+IS+A+PLACEHOLDER&comment=**Phabricator+URL:**+https://phabricator.services.mozilla.com/...&format=__default__).
|
||||
|
||||
You can view these defects in the Diff Detail section of [Phabricator diff 42](https://phabricator.services.mozilla.com/differential/diff/42/).
|
||||
|
|
@ -181,14 +190,15 @@ VALID_NOTICE_MESSAGE = """
|
|||
|
||||
|
||||
---
|
||||
|
||||
If you see a problem in this automated review, [please report it here](https://bugzilla.mozilla.org/enter_bug.cgi?product=Developer+Infrastructure&component=Source+Code+Analysis&short_desc=[Automated+review]+THIS+IS+A+PLACEHOLDER&comment=**Phabricator+URL:**+https://phabricator.services.mozilla.com/...&format=__default__).
|
||||
"""
|
||||
|
||||
VALID_EXTERNAL_TIDY_MESSAGE = """
|
||||
Code analysis found 1 defect in the diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
Code analysis found 1 defect in diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
- 1 defect found by private static analysis
|
||||
|
||||
WARNING: Found 1 issue (warning level) that can be dismissed.
|
||||
WARNING: Found 1 defect (warning level) that can be dismissed.
|
||||
|
||||
You can run this analysis locally with:
|
||||
- For private static analysis, please see [our private docs in Mana](https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=130909687), if you cannot access this resource, ask your reviewer to help you resolve the issue.
|
||||
|
|
@ -203,18 +213,19 @@ You can run this analysis locally with:
|
|||
|
||||
|
||||
---
|
||||
|
||||
If you see a problem in this automated review, [please report it here](https://bugzilla.mozilla.org/enter_bug.cgi?product=Developer+Infrastructure&component=Source+Code+Analysis&short_desc=[Automated+review]+THIS+IS+A+PLACEHOLDER&comment=**Phabricator+URL:**+https://phabricator.services.mozilla.com/...&format=__default__).
|
||||
|
||||
You can view these defects in the Diff Detail section of [Phabricator diff 42](https://phabricator.services.mozilla.com/differential/diff/42/).
|
||||
"""
|
||||
|
||||
FOLLOW_UP_DIFF_MESSAGE = """
|
||||
Code analysis found 2 defects in the diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
Code analysis found 2 defects in diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
- 1 defect found by clang-format
|
||||
- 1 defect found by code coverage analysis
|
||||
1 issue unresolved and 1 issue closed compared to the previous diff [41](https://phabricator.services.mozilla.com/differential/diff/41/).
|
||||
1 defect unresolved and 1 defect closed compared to the previous diff [41](https://phabricator.services.mozilla.com/differential/diff/41/).
|
||||
|
||||
WARNING: Found 2 issues (warning level) that can be dismissed.
|
||||
WARNING: Found 2 defects (warning level) that can be dismissed.
|
||||
|
||||
You can run this analysis locally with:
|
||||
- `./mach clang-format -p dom/test.cpp`
|
||||
|
|
@ -227,10 +238,28 @@ Should they have tests, or are they dead code?
|
|||
|
||||
|
||||
---
|
||||
|
||||
If you see a problem in this automated review, [please report it here](https://bugzilla.mozilla.org/enter_bug.cgi?product=Developer+Infrastructure&component=Source+Code+Analysis&short_desc=[Automated+review]+THIS+IS+A+PLACEHOLDER&comment=**Phabricator+URL:**+https://phabricator.services.mozilla.com/...&format=__default__).\n
|
||||
You can view these defects in the Diff Detail section of [Phabricator diff 42](https://phabricator.services.mozilla.com/differential/diff/42/).
|
||||
"""
|
||||
|
||||
VALID_CLANG_BEFORE_AFTER_MESSAGE = """
|
||||
Code analysis found 1 defect in diff [42](https://phabricator.services.mozilla.com/differential/diff/42/):
|
||||
- 1 defect found by clang-format
|
||||
|
||||
WARNING: Found 1 defect (warning level) that can be dismissed.
|
||||
|
||||
You can run this analysis locally with:
|
||||
- `./mach clang-format -p outside/of/the/patch.cpp`
|
||||
|
||||
|
||||
---
|
||||
|
||||
If you see a problem in this automated review, [please report it here](https://bugzilla.mozilla.org/enter_bug.cgi?product=Developer+Infrastructure&component=Source+Code+Analysis&short_desc=[Automated+review]+THIS+IS+A+PLACEHOLDER&comment=**Phabricator+URL:**+https://phabricator.services.mozilla.com/...&format=__default__).
|
||||
|
||||
You can view these defects in the Diff Detail section of [Phabricator diff 42](https://phabricator.services.mozilla.com/differential/diff/42/).
|
||||
"""
|
||||
|
||||
|
||||
def test_phabricator_clang_tidy(
|
||||
mock_phabricator, phab, mock_try_task, mock_decision_task, mock_task
|
||||
|
|
@ -246,6 +275,7 @@ def test_phabricator_clang_tidy(
|
|||
"another_test.cpp": [41, 42, 43]
|
||||
}
|
||||
revision.files = ["another_test.cpp"]
|
||||
revision.id = 52
|
||||
reporter = PhabricatorReporter({"analyzers": ["clang-tidy"]}, api=api)
|
||||
|
||||
issue = ClangTidyIssue(
|
||||
|
|
@ -281,6 +311,7 @@ def test_phabricator_clang_format(
|
|||
"test.cpp": [41, 42, 43],
|
||||
"dom/test.cpp": [42],
|
||||
}
|
||||
revision.id = 52
|
||||
reporter = PhabricatorReporter({"analyzers": ["clang-format"]}, api=api)
|
||||
|
||||
task = mock_task(ClangFormatTask, "source-test-clang-format")
|
||||
|
|
@ -324,6 +355,7 @@ def test_phabricator_mozlint(
|
|||
"dom/test.cpp": [42],
|
||||
}
|
||||
revision.files = revision.lines.keys()
|
||||
revision.id = 52
|
||||
reporter = PhabricatorReporter({}, api=api)
|
||||
|
||||
issue_flake = MozLintIssue(
|
||||
|
|
@ -406,6 +438,7 @@ def test_phabricator_coverage(
|
|||
"path/to/test.cpp": [0],
|
||||
"dom/test.cpp": [42],
|
||||
}
|
||||
revision.id = 52
|
||||
reporter = PhabricatorReporter({"analyzers": ["coverage"]}, api=api)
|
||||
|
||||
issue = CoverageIssue(
|
||||
|
|
@ -460,6 +493,7 @@ def test_phabricator_clang_tidy_and_coverage(
|
|||
"another_test.cpp": [41, 42, 43],
|
||||
}
|
||||
revision.files = ["test.txt", "test.cpp", "another_test.cpp"]
|
||||
revision.id = 52
|
||||
reporter = PhabricatorReporter(
|
||||
{"analyzers": ["coverage", "clang-tidy"]}, api=api
|
||||
)
|
||||
|
|
@ -576,6 +610,7 @@ def test_phabricator_analyzers(
|
|||
# Always use the same setup, only varies the analyzers
|
||||
revision = Revision.from_try_task(mock_try_task, mock_decision_task, api)
|
||||
revision.lines = {"test.cpp": [0, 41, 42, 43], "dom/test.cpp": [42]}
|
||||
revision.id = 52
|
||||
reporter = PhabricatorReporter(
|
||||
{"analyzers_skipped": analyzers_skipped}, api=api
|
||||
)
|
||||
|
|
@ -662,6 +697,7 @@ def test_phabricator_clang_tidy_build_error(
|
|||
# Add dummy lines diff
|
||||
"test.cpp": [41, 42, 43]
|
||||
}
|
||||
revision.id = 52
|
||||
revision.build_target_phid = "PHID-HMBD-deadbeef12456"
|
||||
|
||||
reporter = PhabricatorReporter({}, api=api)
|
||||
|
|
@ -720,6 +756,7 @@ def test_full_file(
|
|||
"xx.cpp": [123, 124, 125]
|
||||
}
|
||||
revision.files = list(revision.lines.keys())
|
||||
revision.id = 52
|
||||
reporter = PhabricatorReporter(api=api)
|
||||
|
||||
issue = DefaultIssue(
|
||||
|
|
@ -776,6 +813,7 @@ def test_task_failures(mock_phabricator, phab, mock_try_task, mock_decision_task
|
|||
|
||||
with mock_phabricator as api:
|
||||
revision = Revision.from_try_task(mock_try_task, mock_decision_task, api)
|
||||
revision.id = 52
|
||||
reporter = PhabricatorReporter({"analyzers": ["clang-tidy"]}, api=api)
|
||||
|
||||
status = {
|
||||
|
|
@ -802,6 +840,7 @@ def test_extra_errors(
|
|||
revision = Revision.from_try_task(mock_try_task, mock_decision_task, api)
|
||||
revision.lines = {"path/to/file.py": [1, 2, 3]}
|
||||
revision.files = ["path/to/file.py"]
|
||||
revision.id = 52
|
||||
reporter = PhabricatorReporter({}, api=api)
|
||||
|
||||
task = mock_task(MozLintTask, "source-test-mozlint-dummy")
|
||||
|
|
@ -894,6 +933,7 @@ def test_phabricator_notices(mock_phabricator, phab, mock_try_task, mock_decisio
|
|||
# Add dummy lines diff
|
||||
"test.rst": [41, 42, 43],
|
||||
}
|
||||
revision.id = 52
|
||||
reporter = PhabricatorReporter({"analyzers": ["doc-upload"]}, api=api)
|
||||
|
||||
doc_url = "http://gecko-docs.mozilla.org-l1.s3-website.us-west-2.amazonaws.com/59dc75b0-e207-11ea-8fa5-0242ac110004/index.html"
|
||||
|
|
@ -941,10 +981,12 @@ def test_phabricator_tgdiff(mock_phabricator, phab, mock_try_task, mock_decision
|
|||
# Add dummy lines diff
|
||||
"test.rst": [41, 42, 43],
|
||||
}
|
||||
revision.id = 52
|
||||
reporter = PhabricatorReporter({"analyzers": ["doc-upload"]}, api=api)
|
||||
|
||||
doc_url = "http://gecko-docs.mozilla.org-l1.s3-website.us-west-2.amazonaws.com/59dc75b0-e207-11ea-8fa5-0242ac110004/index.html"
|
||||
doc_notice = COMMENT_LINK_TO_DOC.format(diff_id=42, doc_url=doc_url)
|
||||
|
||||
reporter.publish(
|
||||
[],
|
||||
revision,
|
||||
|
|
@ -976,6 +1018,7 @@ def test_phabricator_external_tidy(
|
|||
"another_test.cpp": [41, 42, 43]
|
||||
}
|
||||
revision.files = ["another_test.cpp"]
|
||||
revision.id = 52
|
||||
reporter = PhabricatorReporter({"analyzers": ["clang-tidy-external"]}, api=api)
|
||||
|
||||
issue_clang_diagnostic = ExternalTidyIssue(
|
||||
|
|
@ -1104,6 +1147,7 @@ def test_phabricator_former_diff_comparison(
|
|||
"path/to/test.cpp": [0],
|
||||
"dom/test.cpp": [42],
|
||||
}
|
||||
revision.id = 52
|
||||
reporter = PhabricatorReporter({"analyzers": ["coverage"]}, api=api)
|
||||
|
||||
issues = [
|
||||
|
|
@ -1218,8 +1262,108 @@ def test_phabricator_former_diff_comparison(
|
|||
}
|
||||
]
|
||||
|
||||
# Check the comment hasn't been posted
|
||||
assert phab.comments[51] == [FOLLOW_UP_DIFF_MESSAGE]
|
||||
|
||||
# Clear the environment
|
||||
del os.environ["SPECIAL_NAME"]
|
||||
|
||||
|
||||
def test_phabricator_before_after_comment(
|
||||
monkeypatch,
|
||||
mock_phabricator,
|
||||
phab,
|
||||
mock_try_task,
|
||||
mock_decision_task,
|
||||
mock_task,
|
||||
mock_taskcluster_config,
|
||||
):
|
||||
"""
|
||||
Test Phabricator reporter publication shows all type of issues depending on their existence
|
||||
on the backend while running the before/after feature.
|
||||
|
||||
Two warnings are detected, one is reported because it is a new issue while the other one
|
||||
is marked as existing on the backend (resumed by a line in the footer).
|
||||
"""
|
||||
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.lines = {
|
||||
# Add dummy lines diff
|
||||
"test.txt": [0],
|
||||
"path/to/test.cpp": [0],
|
||||
"dom/test.cpp": [42],
|
||||
}
|
||||
revision.id = 52
|
||||
reporter = PhabricatorReporter({"analyzers": ["coverage"]}, api=api)
|
||||
|
||||
assert revision.before_after_feature is True
|
||||
|
||||
# A new warning issue outside of the patch
|
||||
clang_issue = ClangFormatIssue(
|
||||
mock_task(ClangFormatTask, "source-test-clang-format"),
|
||||
"outside/of/the/patch.cpp",
|
||||
[(42, 42, b"That line is wrong. Good luck debugging")],
|
||||
revision,
|
||||
)
|
||||
clang_issue.validates = lambda: True
|
||||
clang_issue.new_issue = True
|
||||
# A warning already existing on the mozilla-central repository
|
||||
cov_issue = CoverageIssue(
|
||||
mock_task(ZeroCoverageTask, "coverage"),
|
||||
"outside/of/the/patch.txt",
|
||||
42,
|
||||
"Coverage warning",
|
||||
revision,
|
||||
)
|
||||
|
||||
# New issues are publishable by default
|
||||
assert clang_issue.is_publishable() is True
|
||||
assert cov_issue.is_publishable() is False
|
||||
|
||||
# Tag the coverage issue as a new issue (nor unresolved nor closed)
|
||||
reporter.compare_issues = lambda former_diff, issues: ([], [])
|
||||
|
||||
with capture_logs() as cap_logs:
|
||||
issues, patches = reporter.publish(
|
||||
[cov_issue, clang_issue], revision, [], [], []
|
||||
)
|
||||
|
||||
assert cap_logs == [
|
||||
{
|
||||
"event": "Updated Harbormaster build state with issues",
|
||||
"log_level": "info",
|
||||
"nb_lint": 1,
|
||||
"nb_unit": 0,
|
||||
},
|
||||
{
|
||||
"event": "Published phabricator summary",
|
||||
"log_level": "info",
|
||||
},
|
||||
]
|
||||
|
||||
# Check the lint results
|
||||
assert phab.build_messages["PHID-HMBT-test"] == [
|
||||
{
|
||||
"receiver": "PHID-HMBT-test",
|
||||
"lint": [
|
||||
{
|
||||
"code": "invalid-styling",
|
||||
"description": (
|
||||
"WARNING: The change does not follow the C/C++ "
|
||||
"coding style, please reformat\n\n"
|
||||
" lang=c++\n"
|
||||
" That line is wrong. Good luck debugging"
|
||||
),
|
||||
"line": 42,
|
||||
"name": "clang-format",
|
||||
"path": "outside/of/the/patch.cpp",
|
||||
"severity": "warning",
|
||||
},
|
||||
],
|
||||
"type": "work",
|
||||
"unit": [],
|
||||
}
|
||||
]
|
||||
|
||||
assert phab.comments[51] == [VALID_CLANG_BEFORE_AFTER_MESSAGE]
|
||||
|
|
|
|||
|
|
@ -195,3 +195,14 @@ def test_bugzilla_id(mock_revision):
|
|||
# On missing data fallback gracefully
|
||||
del mock_revision.revision["fields"]["bugzilla.bug-id"]
|
||||
assert mock_revision.bugzilla_id is None
|
||||
|
||||
|
||||
def test_revision_before_after(mock_config, mock_revision, mock_taskcluster_config):
|
||||
"""
|
||||
Ensure before/after feature is always run on specific revisions
|
||||
"""
|
||||
mock_taskcluster_config.secrets["BEFORE_AFTER_RATIO"] = 0.5
|
||||
mock_revision.id = 51
|
||||
assert mock_revision.before_after_feature is True
|
||||
mock_revision.id = 42
|
||||
assert mock_revision.before_after_feature is False
|
||||
|
|
|
|||
|
|
@ -4,13 +4,15 @@
|
|||
# file, You can obtain one at http://mozilla.org/MPL/2.0/.
|
||||
|
||||
import os
|
||||
from datetime import datetime
|
||||
from unittest import mock
|
||||
|
||||
import pytest
|
||||
import responses
|
||||
|
||||
from code_review_bot.config import Settings
|
||||
from code_review_bot.revisions import Revision
|
||||
from code_review_bot.tasks.clang_format import ClangFormatTask
|
||||
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.lint import MozLintTask
|
||||
|
|
@ -154,3 +156,72 @@ def test_on_production(mock_config, mock_repositories):
|
|||
assert testing.app_channel == "production"
|
||||
assert testing.taskcluster.local is False
|
||||
assert testing.on_production is True
|
||||
|
||||
|
||||
def test_before_after(mock_taskcluster_config, mock_workflow, mock_task, mock_revision):
|
||||
"""
|
||||
Test the before/after feature running a try task workflow.
|
||||
Issues that are unknown to the backend are tagged as new issues.
|
||||
"""
|
||||
mock_taskcluster_config.secrets = {"BEFORE_AFTER_RATIO": 1}
|
||||
issues = [
|
||||
ClangFormatIssue(
|
||||
mock_task(ClangFormatTask, "source-test-clang-format"),
|
||||
"outside/of/the/patch.cpp",
|
||||
[(42, 42, b"This is a new warning.")],
|
||||
mock_revision,
|
||||
),
|
||||
ClangFormatIssue(
|
||||
mock_task(ClangFormatTask, "source-test-clang-format"),
|
||||
"outside/of/the/patch.cpp",
|
||||
[(42, 42, b"This is a warning known by the backend.")],
|
||||
mock_revision,
|
||||
),
|
||||
]
|
||||
mock_workflow.publish = mock.Mock()
|
||||
mock_workflow.find_issues = mock.Mock()
|
||||
mock_workflow.find_issues.return_value = (
|
||||
issues,
|
||||
# No failure nor notices nor reviewers
|
||||
[],
|
||||
[],
|
||||
[],
|
||||
)
|
||||
mock_workflow.queue_service.task = lambda x: {}
|
||||
mock_workflow.mercurial_repository = None
|
||||
mock_workflow.backend_api.url = "https://backend.test"
|
||||
mock_workflow.backend_api.username = "root"
|
||||
mock_workflow.backend_api.password = "hunter2"
|
||||
for index, hash_val in enumerate(("aaaa", "bbbb")):
|
||||
issues[index].hash = hash_val
|
||||
|
||||
current_date = datetime.now().strftime("%Y-%m-%d")
|
||||
responses.add(
|
||||
responses.GET,
|
||||
f"https://backend.test/v1/issues/mozilla-central/?path=outside%2Fof%2Fthe%2Fpatch.cpp&date={current_date}",
|
||||
json={
|
||||
"count": 2,
|
||||
"previous": None,
|
||||
"next": None,
|
||||
"results": [
|
||||
{"id": "issue 1", "hash": "bbbb"},
|
||||
{"id": "issue 42", "hash": "xxxx"},
|
||||
],
|
||||
},
|
||||
)
|
||||
|
||||
# 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)
|
||||
assert mock_workflow.publish.call_args_list == [
|
||||
mock.call(
|
||||
mock_revision,
|
||||
issues,
|
||||
[],
|
||||
[],
|
||||
[],
|
||||
)
|
||||
]
|
||||
assert issues[0].new_issue is True
|
||||
assert issues[1].new_issue is False
|
||||
|
|
|
|||
|
|
@ -108,6 +108,10 @@ bot:
|
|||
# Boolean option to enable/disable the low coverage warning
|
||||
ZERO_COVERAGE_ENABLED: true
|
||||
|
||||
# Float ratio (between 0.0 and 1.0) defining the chance for a patch to be analyzed with
|
||||
# the before/after feature (filters out known issues and warn about issues outside the patch)
|
||||
BEFORE_AFTER_RATIO: 0.3
|
||||
|
||||
# Connection information to publish issues on the backend
|
||||
# On local development it should be set to the local backend running in Docker
|
||||
backend:
|
||||
|
|
|
|||
Загрузка…
Ссылка в новой задаче