From 80226254ea024e756b9ad5e8a39160405880cbb1 Mon Sep 17 00:00:00 2001 From: Anne Redulla Date: Sun, 10 Sep 2023 22:52:26 +0000 Subject: [PATCH] [ssci] Modify metadata versioning info validation This CL changes what is considered valid versioning info. Instead of both Date and Revision being required if Version was unknown, now only one of Date or Revision has to be specified. Bug: b:277147404 Change-Id: Iedb06e2d55f0cd0ef0a2931013a2a52b15befd75 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4852699 Reviewed-by: Gavin Mak Commit-Queue: Anne Redulla Reviewed-by: Rachael Newitt --- metadata/dependency_metadata.py | 26 +++++++++---- metadata/tests/dependency_metadata_test.py | 43 ++++++++++++++++++++++ metadata/tests/validate_test.py | 12 +++--- tests/presubmit_canned_checks_test.py | 7 ++-- 4 files changed, 72 insertions(+), 16 deletions(-) diff --git a/metadata/dependency_metadata.py b/metadata/dependency_metadata.py index 7c1709ee0..0f5082e3b 100644 --- a/metadata/dependency_metadata.py +++ b/metadata/dependency_metadata.py @@ -68,13 +68,6 @@ class DependencyMetadata: """ required = set(self._MANDATORY_FIELDS) - # The date and revision are required if the version has not - # been specified. - version_value = self._metadata.get(known_fields.VERSION) - if version_value is None or version_util.is_unknown(version_value): - required.add(known_fields.DATE) - required.add(known_fields.REVISION) - # Assume the dependency is shipped if not specified. shipped_value = self._metadata.get(known_fields.SHIPPED) is_shipped = (shipped_value is None @@ -138,6 +131,25 @@ class DependencyMetadata: reason=f"Required field '{field_name}' is missing.") results.append(error) + # At least one of the fields Version, Date or Revision must be + # provided. + version_value = self._metadata.get(known_fields.VERSION) + date_value = self._metadata.get(known_fields.DATE) + revision_value = self._metadata.get(known_fields.REVISION) + if ((not version_value or version_util.is_unknown(version_value)) + and (not date_value or util.is_unknown(date_value)) + and (not revision_value or util.is_unknown(revision_value))): + versioning_fields = [ + known_fields.VERSION, known_fields.DATE, known_fields.REVISION + ] + names = util.quoted( + [field.get_name() for field in versioning_fields]) + error = vr.ValidationError( + reason="Versioning fields are insufficient.", + additional=[f"Provide at least one of [{names}]."], + ) + results.append(error) + # Validate values for all present fields. for field, value in self._metadata.items(): field_result = field.validate(value) diff --git a/metadata/tests/dependency_metadata_test.py b/metadata/tests/dependency_metadata_test.py index a0ec1a20e..aa71d2943 100644 --- a/metadata/tests/dependency_metadata_test.py +++ b/metadata/tests/dependency_metadata_test.py @@ -44,6 +44,30 @@ class DependencyValidationTest(unittest.TestCase): self.assertTrue(isinstance(results[0], vr.ValidationError)) self.assertEqual(results[0].get_reason(), "There is a repeated field.") + def test_versioning_field(self): + """Check that a validation error is returned for insufficient + versioning info.""" + dependency = dm.DependencyMetadata() + dependency.add_entry(known_fields.NAME.get_name(), + "Test metadata missing versioning info") + dependency.add_entry(known_fields.URL.get_name(), + "https://www.example.com") + dependency.add_entry(known_fields.VERSION.get_name(), "N/A") + dependency.add_entry(known_fields.REVISION.get_name(), "unknown") + dependency.add_entry(known_fields.LICENSE.get_name(), "Public Domain") + dependency.add_entry(known_fields.LICENSE_FILE.get_name(), "LICENSE") + dependency.add_entry(known_fields.SECURITY_CRITICAL.get_name(), "no") + dependency.add_entry(known_fields.SHIPPED.get_name(), "no") + + results = dependency.validate( + source_file_dir=os.path.join(_THIS_DIR, "data"), + repo_root_dir=_THIS_DIR, + ) + self.assertEqual(len(results), 1) + self.assertTrue(isinstance(results[0], vr.ValidationError)) + self.assertEqual(results[0].get_reason(), + "Versioning fields are insufficient.") + def test_required_field(self): """Check that a validation error is returned for a missing field.""" dependency = dm.DependencyMetadata() @@ -132,6 +156,25 @@ class DependencyValidationTest(unittest.TestCase): ) self.assertEqual(len(results), 4) + def test_valid_metadata(self): + """Check valid metadata returns no validation issues.""" + dependency = dm.DependencyMetadata() + dependency.add_entry(known_fields.NAME.get_name(), + "Test valid metadata") + dependency.add_entry(known_fields.URL.get_name(), + "https://www.example.com") + dependency.add_entry(known_fields.VERSION.get_name(), "1.0.0") + dependency.add_entry(known_fields.LICENSE.get_name(), "Public Domain") + dependency.add_entry(known_fields.LICENSE_FILE.get_name(), "LICENSE") + dependency.add_entry(known_fields.SECURITY_CRITICAL.get_name(), "no") + dependency.add_entry(known_fields.SHIPPED.get_name(), "no") + + results = dependency.validate( + source_file_dir=os.path.join(_THIS_DIR, "data"), + repo_root_dir=_THIS_DIR, + ) + self.assertEqual(len(results), 0) + if __name__ == "__main__": unittest.main() diff --git a/metadata/tests/validate_test.py b/metadata/tests/validate_test.py index 657f2f5ff..f878afbd9 100644 --- a/metadata/tests/validate_test.py +++ b/metadata/tests/validate_test.py @@ -53,7 +53,7 @@ class ValidateContentTest(unittest.TestCase): source_file_dir=_SOURCE_FILE_DIR, repo_root_dir=_THIS_DIR, ) - self.assertEqual(len(results), 11) + self.assertEqual(len(results), 9) error_count = 0 warning_count = 0 for result in results: @@ -61,7 +61,7 @@ class ValidateContentTest(unittest.TestCase): error_count += 1 else: warning_count += 1 - self.assertEqual(error_count, 9) + self.assertEqual(error_count, 7) self.assertEqual(warning_count, 2) @@ -91,7 +91,7 @@ class ValidateFileTest(unittest.TestCase): filepath=_INVALID_METADATA_FILEPATH, repo_root_dir=_THIS_DIR, ) - self.assertEqual(len(results), 11) + self.assertEqual(len(results), 9) error_count = 0 warning_count = 0 for result in results: @@ -99,7 +99,7 @@ class ValidateFileTest(unittest.TestCase): error_count += 1 else: warning_count += 1 - self.assertEqual(error_count, 9) + self.assertEqual(error_count, 7) self.assertEqual(warning_count, 2) @@ -135,10 +135,10 @@ class CheckFileTest(unittest.TestCase): ) # TODO(aredulla): update this test once validation errors can be # returned as errors. Bug: b/285453019. - # self.assertEqual(len(errors), 9) + # self.assertEqual(len(errors), 7) # self.assertEqual(len(warnings), 2) self.assertEqual(len(errors), 0) - self.assertEqual(len(warnings), 11) + self.assertEqual(len(warnings), 9) if __name__ == "__main__": diff --git a/tests/presubmit_canned_checks_test.py b/tests/presubmit_canned_checks_test.py index ebe36252e..7fdae7648 100644 --- a/tests/presubmit_canned_checks_test.py +++ b/tests/presubmit_canned_checks_test.py @@ -390,13 +390,14 @@ class ChromiumDependencyMetadataCheckTest(unittest.TestCase): results = presubmit_canned_checks.CheckChromiumDependencyMetadata( input_api, MockOutputApi()) - # There should be 10 results due to + # There should be 9 results due to # - missing 5 mandatory fields: Name, URL, Version, License, and # Security Critical - # - missing 4 required fields: Date, Revision, License File, and + # - 1 error for insufficent versioning info + # - missing 2 required fields: License File, and # License Android Compatible # - Shipped should be only 'yes' or 'no'. - self.assertEqual(len(results), 10) + self.assertEqual(len(results), 9) # Check each presubmit result is associated with the test file. for result in results: