PR#230: Fix the test suite - releng/fedora-module-defaults

releng / fedora-module-defaults

#230 Fix the test suite

Merged 2 years ago by ppisar. Opened 2 years ago by fivaldi.

releng/ fivaldi/fedora-module-defaults fix_test_suite into main

Filip Valder • 2 years ago

1fa54e2

tests/exclusions.txt

file modified

		`@@ -28,3 +28,4 @@`
		`tests/module.yaml`
		`tests/modulewithnames.yaml`
		`tests/nodejs.yaml`
		`+ tests/obsoletes/nodejs:11.yaml`

tests/obsoletes/nodejs:11.yaml ~~tests/obsoletes/nodejs.yaml~~

file renamed

file was moved with no change to the file

tests/validate.py

file modified

+52 -36

		`@@ -79,8 +79,24 @@`

		`# Filenames must match their contents`
		`expected_name = os.path.basename(filename).rsplit(".", maxsplit=1)[0]`
		`+ if obsoletes:`
		`+ if ":" not in expected_name:`
		`+ error(`
		`+ "Module obsoletes filename must be in the form of "`
		`+ "<name>:<old_stream>.yaml"`
		`+ )`
		`+ return False, None`
		`+ expected_module_name, expected_stream = expected_name.rsplit(":", maxsplit=1)`
		`+ if (expected_module_name != mmd.props.module_name`
		`+ or expected_stream != mmd.props.module_stream):`
		`+ error(`
		`+ 'Module name/stream "{}:{}" doesn\'t match filename "{}.yaml"'.format(`
		`+ mmd.props.module_name, mmd.props.module_stream, expected_name`
		`+ )`
		`+ )`
		`+ return False, None`

		`- if expected_name != mmd.props.module_name:`
		`+ elif expected_name != mmd.props.module_name:`
		`error(`
		`'Module name "{}" doesn\'t match filename "{}.yaml"'.format(`
		`mmd.props.module_name, expected_name`
		`@@ -123,6 +139,7 @@`
		`script_dir = os.path.dirname(os.path.realpath(__file__))`
		`defaults_dir = os.path.abspath(os.path.join(script_dir, '..'))`
		`overrides_dir = os.path.join(defaults_dir, 'overrides')`
		`+ obsoletes_dir = os.path.abspath(os.path.join(script_dir, '..', 'obsoletes'))`

		`# Get the repo we're running in`
		`repo = git.Repo(defaults_dir, search_parent_directories=True)`
		`@@ -153,6 +170,12 @@`
		`else:`
		`defaults_files.append(x)`

		`+ print("\nValidation of modulemd documents:\n"`
		`+ "=================================")`
		`+`
		`+ if defaults_files:`
		`+ print("\nModule defaults:\n"`
		`+ "----------------")`
		`# Validate module defaults files`
		`for file in defaults_files:`
		`(valid, _) = do_validate(file)`
		`@@ -160,6 +183,9 @@`
		`error("{} failed to validate".format(file))`
		`result = os.EX_DATAERR`

		`+ if obsoletes_test_files:`
		`+ print("\nModule obsoletes (testing):\n"`
		`+ "---------------------------")`
		`# Validate module obsoletes test files`
		`for file in obsoletes_test_files:`
		`(valid, _) = do_validate(file, obsoletes=True)`
		`@@ -170,6 +196,8 @@`
		`if result == os.EX_DATAERR:`
		`return result`

		`+ print("\nDefault streams (Runtime):\n"`
		`+ "==========================")`
		`# For sanity's sake, also do a merge of all the defaults to make sure no`
		`# conflicts arise that weren't detected by the above tests. This should be`
		`# impossible.`
		`@@ -177,11 +205,6 @@`
		`idx_runtime = Modulemd.ModuleIndex()`
		`idx_runtime.update_from_defaults_directory(path=defaults_dir,`
		`strict=True)`
		`- # for obsoletes tests, because simple index copy is not possible`
		`- # due to: GObject descendants' instances are non-copyable`
		`- idx_tests = Modulemd.ModuleIndex()`
		`- idx_tests.update_from_defaults_directory(path=defaults_dir,`
		`- strict=True)`
		`except GLib.Error as e:`
		`error("Could not merge all defaults: {}".format(e.message))`
		`result = os.EX_DATAERR`
		`@@ -189,11 +212,11 @@`
		`if result == os.EX_OK:`
		`info("Merging all of the documents encountered no errors.")`

		`- print("\nDefault streams (Runtime):\n"`
		`- "==========================")`
		`for m, s in idx_runtime.get_default_streams().items():`
		`print("{}:{}".format(m, s))`

		`+ print("\nDefault streams (Buildroot):\n"`
		`+ "============================")`
		`try:`
		`idx_buildroot = Modulemd.ModuleIndex()`
		`idx_buildroot.update_from_defaults_directory(path=defaults_dir,`
		`@@ -206,44 +229,37 @@`
		`if result == os.EX_OK:`
		`info("Merging all of the documents encountered no errors.")`

		`- print("\nDefault streams (Buildroot):\n"`
		`- "============================")`
		`for m, s in idx_buildroot.get_default_streams().items():`
		`print("{}:{}".format(m, s))`

		`- # Try merging TEST module obsoletes into the module defaults index.`
		`- for file in obsoletes_test_files:`
		`- (_, mmd) = do_validate(file, obsoletes=True)`
		`- idx_tests.add_obsoletes(mmd)`
		`+ print("\nObsoletes:\n"`
		`+ "==========")`

		`- print("\nObsoletes (tests):\n"`
		`- "==================")`
		`- for module_name in idx_tests.get_module_names():`
		`- module = idx_tests.get_module(module_name)`
		`- obsoletes = module.get_obsoletes()`
		`- if not obsoletes:`
		`- continue`
		`- if len(obsoletes) > 1:`
		`- result = os.EX_DATAERR`
		`- break`
		`- print("{}:{}".format(module_name, obsoletes[0].props.module_stream))`
		`+ try:`
		`+ idx_obsoletes = Modulemd.ModuleIndex()`
		`+ idx_obsoletes.update_from_defaults_directory(path=obsoletes_dir,`
		`+ strict=True)`
		`+ # Try merging module obsoletes into the module obsoletes index.`
		`+ for file in obsoletes_files:`
		`+ (_, mmd) = do_validate(file, obsoletes=True)`
		`+ if not mmd:`
		`+ result = os.EX_DATAERR`
		`+ break`
		`+ idx_obsoletes.add_obsoletes(mmd)`
		`+ except GLib.Error as e:`
		`+ error("Could not merge all obsoletes: {}".format(e.message))`
		`+ result = os.EX_DATAERR`

		`- # Try merging module obsoletes into the module defaults index.`
		`- for file in obsoletes_files:`
		`- (_, mmd) = do_validate(file, obsoletes=True)`
		`- idx_runtime.add_obsoletes(mmd)`
		`+ if result == os.EX_OK:`
		`+ info("Merging all of the documents encountered no errors.")`

		`- print("\nObsoletes:\n"`
		`- "==========")`
		`- for module_name in idx_runtime.get_module_names():`
		`- module = idx_runtime.get_module(module_name)`
		`+ for module_name in idx_obsoletes.get_module_names():`
		`+ module = idx_obsoletes.get_module(module_name)`
		`obsoletes = module.get_obsoletes()`
		`if not obsoletes:`
		`- continue`
		`- if len(obsoletes) > 1:`
		`+ error("Module {} has no obsoletes.".format(module_name))`
		`result = os.EX_DATAERR`
		`break`
		`- print("{}:{}".format(module_name, obsoletes[0].props.module_stream))`

		`return result`

fivaldi commented 2 years ago

Move the prints so it's clear in which phase we're in.
Support obsoletes module:stream.yaml format.
Merge obsoletes into a separate index (don't mix with the defaults).
Exclude test obsoletes.
Pre-tested with perl:5.30.yaml, see PR#229.

Signed-off-by: Filip Valder fvalder@redhat.com

fivaldi commented 2 years ago

@ppisar Please can you check this one? Then we can continue the PR#229.

Thanks a lot.

pretty please pagure-ci rebuild

2 years ago

Metadata Update from @ppisar:
- Request assigned

2 years ago

ppisar commented 2 years ago

If you now require an obsoletes file name to contain a stream, then you should check for it in tests/validate.py. See code under "Filenames must match their contents" comment. You check for a module name with "expected_name != mmd.props.module_name", but there is no check for the stream in the file name.

Also an error message ('Module name "{}" doesn\'t match filename "{}.yaml"') under "expected_name != mmd.props.module_name" condition does not match the name:stream.yaml format. The error message must be adjusted. Don't forget that defaults still use name.yaml format.

Otherwise it looks good. Could you please address the two issues?

rebased onto 1fa54e2

2 years ago

fivaldi commented 2 years ago

Thanks for review & comment, I fixed it like this:

--- a/tests/validate.py
+++ b/tests/validate.py
@@ -86,9 +86,17 @@ def do_validate(filename, obsoletes=False):
                 "<name>:<old_stream>.yaml"
             )
             return False, None
-        expected_name = expected_name.rsplit(":", maxsplit=1)[0]
+        expected_module_name, expected_stream = expected_name.rsplit(":", maxsplit=1)
+        if (expected_module_name != mmd.props.module_name
+                or expected_stream != mmd.props.module_stream):
+            error(
+                'Module name/stream "{}:{}" doesn\'t match filename "{}.yaml"'.format(
+                    mmd.props.module_name, mmd.props.module_stream, expected_name
+                )
+            )
+            return False, None

-    if expected_name != mmd.props.module_name:
+    elif expected_name != mmd.props.module_name:
         error(
             'Module name "{}" doesn\'t match filename "{}.yaml"'.format(
                 mmd.props.module_name, expected_name
@@ -234,6 +242,9 @@ def main():
         # Try merging module obsoletes into the module obsoletes index.
         for file in obsoletes_files:
             (_, mmd) = do_validate(file, obsoletes=True)
+            if not mmd:
+                result = os.EX_DATAERR
+                break
             idx_obsoletes.add_obsoletes(mmd)
     except GLib.Error as e:
         error("Could not merge all obsoletes: {}".format(e.message))

Pull-Request has been merged by ppisar

2 years ago

ppisar commented 2 years ago

Applied to all Fedoras.