Run `depot_tools` canned checks after Devtools checks in PRESUBMIT
If you have whitespace or a missing EOL, ESLint would automatically
fix these issues. However, the canned checks ran before ESLint. Thus,
the canned checks detected whitespace/EOL issues and would report
these problems. This would be confusing, but wouldn't be an issue in
isolation as you would have changed files that need to be checked in.
However, the presubmit then claims "Do you want to proceed regardless
of PRESUBMIT errors", as the canned checks can be suppressed by default.
This would then confuse engineers, as they are uploading, the presubmit
would report errors that had already been fixed and would (understandably)
skip the presubmit. They would then see CQ fail after a while, which
would be a while later.
R=jacktfranklin@chromium.org
Change-Id: I5bd55fbf441eed0fe1bcd37e87b03699c9774d40
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2409980
Auto-Submit: Tim van der Lippe <tvanderlippe@chromium.org>
Commit-Queue: Jack Franklin <jacktfranklin@chromium.org>
Reviewed-by: Jack Franklin <jacktfranklin@chromium.org>
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
index 296c95b..74ded97 100644
--- a/PRESUBMIT.py
+++ b/PRESUBMIT.py
@@ -396,17 +396,28 @@
return results
+def _RunCannedChecks(input_api, output_api):
+ results = []
+ results.extend(
+ input_api.canned_checks.CheckOwnersFormat(input_api, output_api))
+ results.extend(input_api.canned_checks.CheckOwners(input_api, output_api))
+ results.extend(
+ input_api.canned_checks.CheckChangeHasNoCrAndHasOnlyOneEol(
+ input_api, output_api))
+ results.extend(
+ input_api.canned_checks.CheckChangeHasNoStrayWhitespace(
+ input_api, output_api))
+ results.extend(
+ input_api.canned_checks.CheckGenderNeutral(input_api, output_api))
+ return results
+
+
def _CommonChecks(input_api, output_api):
"""Checks common to both upload and commit."""
results = []
results.extend(
input_api.canned_checks.CheckAuthorizedAuthor(
input_api, output_api, bot_allowlist=[AUTOROLL_ACCOUNT]))
- results.extend(input_api.canned_checks.CheckOwnersFormat(input_api, output_api))
- results.extend(input_api.canned_checks.CheckOwners(input_api, output_api))
- results.extend(input_api.canned_checks.CheckChangeHasNoCrAndHasOnlyOneEol(input_api, output_api))
- results.extend(input_api.canned_checks.CheckChangeHasNoStrayWhitespace(input_api, output_api))
- results.extend(input_api.canned_checks.CheckGenderNeutral(input_api, output_api))
results.extend(_CheckBuildGN(input_api, output_api))
results.extend(_CheckExperimentTelemetry(input_api, output_api))
results.extend(_CheckGeneratedFiles(input_api, output_api))
@@ -417,6 +428,16 @@
results.extend(_CheckOptimizeSVGHashes(input_api, output_api))
results.extend(_CheckChangesAreExclusiveToDirectory(input_api, output_api))
results.extend(_CheckComponentBridgesUpToDate(input_api, output_api))
+ # Run the canned checks from `depot_tools` after the custom DevTools checks.
+ # The canned checks for example check that lines have line endings. The
+ # DevTools presubmit checks automatically fix these issues. If we would run
+ # the canned checks before the DevTools checks, they would erroneously conclude
+ # that there are issues in the code. Since the canned checks are allowed to be
+ # ignored, a confusing message is shown that asks if the failed presubmit can
+ # be continued regardless. By fixing the issues before we reach the canned checks,
+ # we don't show the message to suppress these errors, which would otherwise be
+ # causing CQ to fail.
+ results.extend(_RunCannedChecks(input_api, output_api))
results.extend(_CheckNoUncheckedFiles(input_api, output_api))
results.extend(_CheckForTooLargeFiles(input_api, output_api))
return results