Addressed copilot review suggests and fixed skipped tests

Author: VisLab

hed/errors/error_reporter.py

@@ -286,22 +286,23 @@ def filter_issues_by_severity(issues_list: list[dict], severity: int) -> list[di
             list[dict]: A list of dictionaries containing the issue list after filtering by severity.
 
         """
-        return [issue for issue in issues_list if issue["severity"] <= severity]
+        return [issue for issue in issues_list if issue.get("severity", ErrorSeverity.ERROR) <= severity]
 
     @staticmethod
     def separate_issues(issues_list: list[dict]) -> tuple[list[dict], list[dict]]:
         """Separate a list of issues into errors and warnings.
 
         Parameters:
-            issues_list (list[dict]): A list of issue dictionaries, each containing a 'severity' key.
+            issues_list (list[dict]): A list of issue dictionaries. The 'severity' key is
+                optional; issues that omit it are treated as errors (ErrorSeverity.ERROR).
 
         Returns:
             tuple[list[dict], list[dict]]: A tuple of (errors, warnings) where errors contains
                 issues with severity <= ErrorSeverity.ERROR and warnings contains issues with
-                severity >= ErrorSeverity.WARNING.
+                severity > ErrorSeverity.ERROR.
         """
-        errors = [issue for issue in issues_list if issue["severity"] <= ErrorSeverity.ERROR]
-        warnings = [issue for issue in issues_list if issue["severity"] >= ErrorSeverity.WARNING]
+        errors = [issue for issue in issues_list if issue.get("severity", ErrorSeverity.ERROR) <= ErrorSeverity.ERROR]
+        warnings = [issue for issue in issues_list if issue.get("severity", ErrorSeverity.ERROR) > ErrorSeverity.ERROR]
         return errors, warnings
 
     @staticmethod

hed/scripts/schema_script_util.py

@@ -32,16 +32,16 @@ def _is_prerelease_partner(base_schema) -> bool:
     return hed_cache.get_hed_version_path(with_standard, check_prerelease=False) is None
 
 
-def validate_schema_object(base_schema, schema_name, check_warnings=True):
-    """Validate a schema object by checking non-warning compliance and roundtrip conversion.
+def validate_schema_object(base_schema, schema_name, check_warnings=False):
+    """Validate a schema object by checking compliance and roundtrip conversion.
 
-    Tests the schema for non-warning compliance issues and validates that it can be successfully
+    Tests the schema for compliance issues and validates that it can be successfully
     converted to and reloaded from all four formats (MEDIAWIKI, XML, JSON, TSV).
 
     Parameters:
         base_schema (HedSchema): The schema object to validate.
         schema_name (str): The name/path of the schema for error reporting.
-        check_warnings (bool): If True, include warnings in the validation. Default is True.
+        check_warnings (bool): If True, include warnings in the validation. Default is False.
 
     Returns:
         list: A list of validation issue strings. Empty if no issues found.
@@ -81,14 +81,14 @@ def validate_schema(file_path, check_warnings=False):
 
     Loads the schema from file, checks the file extension is lowercase,
     and validates the schema object for compliance errors and roundtrip conversion.
-    Does not check for warnings.
 
     Parameters:
         file_path (str): The path to the schema file to validate.
             If loading a TSV file, this should be a single filename where:
             Template: basename.tsv, where files are named basename_Struct.tsv, basename_Tag.tsv, etc.
             Alternatively, you can point to a directory containing the .tsv files.
         check_warnings (bool): If True, include warnings in the validation. Default is False.
+
     Returns:
         list: A list of validation issue strings. Empty if no issues found.
     """

tests/errors/test_error_reporter.py

@@ -339,3 +339,10 @@ def test_original_list_unchanged(self):
         issues = [self._make_issue(ErrorSeverity.ERROR), self._make_issue(ErrorSeverity.WARNING)]
         ErrorHandler.separate_issues(issues)
         self.assertEqual(len(issues), 2)
+
+    def test_missing_severity_treated_as_error(self):
+        """Issues without a 'severity' key should be treated as errors, not raise KeyError."""
+        issues = [{"message": "no severity"}, self._make_issue(ErrorSeverity.WARNING)]
+        errors, warnings = ErrorHandler.separate_issues(issues)
+        self.assertEqual(len(errors), 1, "Issue missing severity should default to ERROR")
+        self.assertEqual(len(warnings), 1)

tests/scripts/test_hed_convert_schema.py

@@ -1,12 +1,13 @@
-import unittest
-import shutil
+import contextlib
 import copy
+import io
 import os
+import shutil
+import unittest
 from hed import load_schema, load_schema_version
 from hed.schema import HedSectionKey, HedKey
 from hed.scripts.schema_script_util import add_extension
 from hed.scripts.hed_convert_schema import convert_and_update
-import contextlib
 
 
 class TestConvertAndUpdate(unittest.TestCase):
@@ -25,7 +26,7 @@ def test_schema_conversion_and_update(self):
 
         # Assume filenames updated includes just the original schema file for simplicity
         filenames = [original_name]
-        with contextlib.redirect_stdout(None):
+        with contextlib.redirect_stdout(io.StringIO()):
             result = convert_and_update(filenames, set_ids=False)
 
         # Verify no error from convert_and_update and the correct schema version was saved
@@ -43,7 +44,7 @@ def test_schema_conversion_and_update(self):
         schema.save_as_dataframes(tsv_filename)
 
         filenames = [os.path.join(tsv_filename, "test_schema_Tag.tsv")]
-        with contextlib.redirect_stdout(None):
+        with contextlib.redirect_stdout(io.StringIO()):
             result = convert_and_update(filenames, set_ids=False)
 
         # Verify no error from convert_and_update and the correct schema version was saved
@@ -72,7 +73,7 @@ def test_schema_adding_tag(self):
 
         # Assume filenames updated includes just the original schema file for simplicity
         filenames = [add_extension(basename, ".mediawiki")]
-        with contextlib.redirect_stdout(None):
+        with contextlib.redirect_stdout(io.StringIO()):
             result = convert_and_update(filenames, set_ids=False)
         self.assertEqual(result, 0)
 
@@ -81,7 +82,7 @@ def test_schema_adding_tag(self):
         self.assertTrue(x)
         self.assertEqual(schema_reloaded, schema_edited)
 
-        with contextlib.redirect_stdout(None):
+        with contextlib.redirect_stdout(io.StringIO()):
             result = convert_and_update(filenames, set_ids=True)
         self.assertEqual(result, 0)
 

tests/scripts/test_script_util.py

@@ -1,5 +1,6 @@
 import contextlib
 import copy
+import io
 import unittest
 import os
 import shutil
@@ -110,7 +111,7 @@ def test_mixed_file_types(self):
             },
             "other_schema": {".xml": "other_schema.xml"},
         }
-        with contextlib.redirect_stdout(None):
+        with contextlib.redirect_stdout(io.StringIO()):
             result = sort_base_schemas(filenames)
         self.assertEqual(dict(result), expected)
 
@@ -121,7 +122,7 @@ def test_tsv_in_correct_subfolder(self):
             os.path.normpath("hedtsv/wrong_folder/wrong_name_Tag.tsv"),  # Should be ignored
         ]
         expected = {"test_schema": {".tsv": os.path.normpath("hedtsv/test_schema")}}
-        with contextlib.redirect_stdout(None):
+        with contextlib.redirect_stdout(io.StringIO()):
             result = sort_base_schemas(filenames)
         self.assertEqual(dict(result), expected)
 
@@ -134,7 +135,7 @@ def test_tsv_in_correct_subfolder2(self):
         expected = {
             os.path.normpath("prerelease/test_schema"): {".tsv": os.path.normpath("prerelease/hedtsv/test_schema")}
         }
-        with contextlib.redirect_stdout(None):
+        with contextlib.redirect_stdout(io.StringIO()):
             result = sort_base_schemas(filenames)
         self.assertEqual(dict(result), expected)
 
@@ -144,14 +145,14 @@ def test_ignored_files(self):
             os.path.normpath("not_hedtsv/test_schema/test_schema_Tag.tsv"),  # Should be ignored
         ]
         expected = {"test_schema": {".mediawiki": "test_schema.mediawiki"}}
-        with contextlib.redirect_stdout(None):
+        with contextlib.redirect_stdout(io.StringIO()):
             result = sort_base_schemas(filenames)
         self.assertEqual(dict(result), expected)
 
     def test_empty_input(self):
         filenames = []
         expected = {}
-        with contextlib.redirect_stdout(None):
+        with contextlib.redirect_stdout(io.StringIO()):
             result = sort_base_schemas(filenames)
         self.assertEqual(dict(result), expected)
 
@@ -173,7 +174,7 @@ def test_case_insensitive_extensions(self):
                 "case_test_schema": {".mediawiki": "case_test_schema.MEDIAWIKI"},
                 "case_other_schema": {".xml": "case_other_schema.XML"},
             }
-            with contextlib.redirect_stdout(None):
+            with contextlib.redirect_stdout(io.StringIO()):
                 result = sort_base_schemas(filenames)
             self.assertEqual(dict(result), expected)
         finally:
@@ -198,21 +199,21 @@ def test_error_no_error(self):
         schema = load_schema_version("8.4.0")
         schema.save_as_xml(os.path.join(self.base_path, self.basename + ".xml"))
         schema.save_as_dataframes(os.path.join(self.base_path, "hedtsv", self.basename))
-        with contextlib.redirect_stdout(None):
+        with contextlib.redirect_stdout(io.StringIO()):
             issues = validate_all_schema_formats(os.path.join(self.base_path, self.basename))
         self.assertTrue(issues)
         self.assertEqual(issues[0], "Error loading schema: No such file or directory")
         schema.save_as_mediawiki(os.path.join(self.base_path, self.basename + ".mediawiki"))
         schema.save_as_json(os.path.join(self.base_path, self.basename + ".json"))
 
-        with contextlib.redirect_stdout(None):
+        with contextlib.redirect_stdout(io.StringIO()):
             self.assertEqual(validate_all_schema_formats(os.path.join(self.base_path, self.basename)), [])
 
         schema_incorrect = load_schema_version("8.2.0")
         schema_incorrect.save_as_dataframes(os.path.join(self.base_path, "hedtsv", self.basename))
 
         # Validate and expect errors
-        with contextlib.redirect_stdout(None):
+        with contextlib.redirect_stdout(io.StringIO()):
             issues = validate_all_schema_formats(os.path.join(self.base_path, self.basename))
         self.assertTrue(issues)
         # self.assertIn("Error loading schema: No columns to parse from file", issues[0])
@@ -226,7 +227,7 @@ def tearDownClass(cls):
 class TestValidateSchema(unittest.TestCase):
     def test_load_invalid_extension(self):
         # Verify capital letters fail validation
-        with contextlib.redirect_stdout(None):
+        with contextlib.redirect_stdout(io.StringIO()):
             self.assertIn("Only fully lowercase extensions ", validate_schema("does_not_matter.MEDIAWIKI")[0])
             self.assertIn("Only fully lowercase extensions ", validate_schema("does_not_matter.Mediawiki")[0])
             self.assertIn("Only fully lowercase extensions ", validate_schema("does_not_matter.XML")[0])
@@ -256,7 +257,7 @@ def test_uppercase_extension_policy_enforcement(self):
             self.assertEqual(schema_files["policy_test"][".xml"], uppercase_file)
 
             # Step 2: validate_all_schemas should use actual path and reject per policy
-            with contextlib.redirect_stdout(None):
+            with contextlib.redirect_stdout(io.StringIO()):
                 issues = validate_all_schemas(schema_files)
 
             # Should get policy violation, not FileNotFoundError
@@ -284,25 +285,31 @@ def setUpClass(cls):
 
     def test_clean_schema_check_warnings_false(self):
         """A fully compliant schema produces no issues with check_warnings=False."""
-        with contextlib.redirect_stdout(None):
+        with contextlib.redirect_stdout(io.StringIO()):
             issues = validate_schema_object(self.clean_schema, "test", check_warnings=False)
         self.assertEqual(issues, [])
 
     def test_clean_schema_check_warnings_true(self):
         """A fully compliant schema produces no issues with check_warnings=True."""
-        with contextlib.redirect_stdout(None):
+        with contextlib.redirect_stdout(io.StringIO()):
             issues = validate_schema_object(self.clean_schema, "test", check_warnings=True)
         self.assertEqual(issues, [])
 
     def test_warning_schema_check_warnings_true_reports_issues(self):
         """A prerelease version generates a warning that is reported when check_warnings=True."""
-        with contextlib.redirect_stdout(None):
+        with contextlib.redirect_stdout(io.StringIO()):
             issues = validate_schema_object(self.warning_schema, "test", check_warnings=True)
+        combined = "\n".join(issues)
         self.assertTrue(issues, "Expected at least one issue for prerelease version warning")
+        self.assertIn(
+            "SCHEMA_PRERELEASE_VERSION_USED",
+            combined,
+            "Expected SCHEMA_PRERELEASE_VERSION_USED warning code in output for prerelease version",
+        )
 
     def test_warning_schema_check_warnings_false_suppresses_warnings(self):
         """Warnings are suppressed and validation passes when check_warnings=False."""
-        with contextlib.redirect_stdout(None):
+        with contextlib.redirect_stdout(io.StringIO()):
             issues = validate_schema_object(self.warning_schema, "test", check_warnings=False)
         self.assertEqual(issues, [])
 
@@ -314,7 +321,7 @@ def test_validate_schema_default_is_warnings_false(self):
             "schema_tests",
             "HED8.2.0.mediawiki",
         )
-        with contextlib.redirect_stdout(None):
+        with contextlib.redirect_stdout(io.StringIO()):
             default_issues = validate_schema(schema_path)
             explicit_issues = validate_schema(schema_path, check_warnings=False)
         self.assertEqual(default_issues, explicit_issues)