From a60883e901e5bc78a3ebccdca7c01d00e2390536 Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Tue, 25 Feb 2025 16:11:34 -0800 Subject: [PATCH] metadata: Fix metadata validator error reporting Validate errors should be returned as errors (as warnings are ignored), not warnings Once the following CL's have been submitted there will be 0 presubmit errors or warnings. * https://crrev.com/c/6284506 * https://crrev.com/c/6296486 * https://crrev.com/c/6290266 * https://crrev.com/c/6296606 * https://crrev.com/c/6297263 * https://crrev.com/c/6296425 * https://crrev.com/c/6290667 * https://crrev.com/c/6287813 * https://crrev.com/c/6289887 * https://crrev.com/c/6290124 Bug: 285453019 Change-Id: I3448435dcb0505722a2c68476ef9d752a6614533 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6296579 Reviewed-by: Rachael Newitt Commit-Queue: Jordan Brown Commit-Queue: Rachael Newitt Auto-Submit: Jordan Brown --- metadata/tests/validate_test.py | 16 ++++------------ metadata/validate.py | 14 ++------------ 2 files changed, 6 insertions(+), 24 deletions(-) diff --git a/metadata/tests/validate_test.py b/metadata/tests/validate_test.py index 3ab622382..39e4cd8d7 100644 --- a/metadata/tests/validate_test.py +++ b/metadata/tests/validate_test.py @@ -114,12 +114,8 @@ class CheckFileTest(unittest.TestCase): filepath=os.path.join(_THIS_DIR, "data", "MISSING.chromium"), repo_root_dir=_THIS_DIR, ) - # TODO(aredulla): update this test once validation errors can be - # returned as errors. Bug: b/285453019. - # self.assertEqual(len(errors), 1) - # self.assertEqual(len(warnings), 0) - self.assertEqual(len(errors), 0) - self.assertEqual(len(warnings), 1) + self.assertEqual(len(errors), 1) + self.assertEqual(len(warnings), 0) def test_valid(self): # Check file with valid content (no errors or warnings). @@ -136,12 +132,8 @@ class CheckFileTest(unittest.TestCase): filepath=_INVALID_METADATA_FILEPATH, repo_root_dir=_THIS_DIR, ) - # TODO(aredulla): update this test once validation errors can be - # returned as errors. Bug: b/285453019. - # self.assertEqual(len(errors), 7) - # self.assertEqual(len(warnings), 2) - self.assertEqual(len(errors), 0) - self.assertEqual(len(warnings), 9) + self.assertEqual(len(errors), 7) + self.assertEqual(len(warnings), 2) class ValidationResultTest(unittest.TestCase): diff --git a/metadata/validate.py b/metadata/validate.py index b58fb77f4..782c821b5 100644 --- a/metadata/validate.py +++ b/metadata/validate.py @@ -150,17 +150,7 @@ def check_file( error_messages = [] warning_messages = [] for result in results: - # TODO(aredulla): Actually distinguish between validation errors - # and warnings. The quality of metadata is currently being - # uplifted, but is not yet guaranteed to pass validation. So for - # now, all validation results will be returned as warnings so - # CLs are not blocked by invalid metadata in presubmits yet. - # Bug: b/285453019. - if result.is_fatal(): - message = result.get_message(prescript=_TRANSITION_PRESCRIPT, - width=60) - else: - message = result.get_message(width=60) - warning_messages.append(message) + target = error_messages if result.is_fatal() else warning_messages + target.append(result.get_message(width=60)) return error_messages, warning_messages