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: