#12 Add tests for bad input files parsing and fix issues found by tests in the modulemd library.
Closed 7 years ago by nphilipp. Opened 7 years ago by jkaluza.
jkaluza/modulemd master  into  master

file modified
+29 -15
@@ -89,12 +89,14 @@ 

          :raises ValueError: If the metadata is invalid or unsupported.

          """

          yml = yaml.safe_load(s)

-         if yml["document"] != "modulemd":

+         if "document" not in yml or yml["document"] != "modulemd":

              raise ValueError("The supplied data isn't a valid modulemd document")

+         if "version" not in yml:

+             raise ValueError("version is required")

          if yml["version"] not in supported_mdversions:

              raise ValueError("The supplied metadata version isn't supported")

          self.mdversion = yml["version"]

-         if not "data" in yml:

+         if "data" not in yml or not isinstance(yml["data"], dict):

              return

          if "name" in yml["data"]:

              self.name = yml["data"]["name"]
@@ -106,17 +108,24 @@ 

              self.summary = yml["data"]["summary"]

          if "description" in yml["data"]:

              self.description = str(yml["data"]["description"]).strip()

-         if ("license" in yml["data"] and yml["data"]["license"]

-             and "module" in yml["data"]["license"]):

+         if ("license" in yml["data"]

+                 and isinstance(yml["data"]["license"], dict)

+                 and "module" in yml["data"]["license"]

+                 and yml["data"]["license"]["module"]):

              self.module_licenses = set(yml["data"]["license"]["module"])

-         if yml["data"]["license"] and "content" in yml["data"]["license"]:

+         if ("license" in yml["data"]

+                 and isinstance(yml["data"]["license"], dict)

+                 and "content" in yml["data"]["license"]):

              self.content_licenses = set(yml["data"]["license"]["content"])

-         if "dependencies" in yml["data"]:

-             if "buildrequires" in yml["data"]["dependencies"]:

+         if ("dependencies" in yml["data"]

+                 and isinstance(yml["data"]["dependencies"], dict)):

+             if ("buildrequires" in yml["data"]["dependencies"]

+                     and yml["data"]["dependencies"]["buildrequires"]):

                  self.buildrequires = yml["data"]["dependencies"]["buildrequires"]

-             if "requires" in yml["data"]["dependencies"]:

+             if ("requires" in yml["data"]["dependencies"]

+                     and yml["data"]["dependencies"]["requires"]):

                  self.requires = yml["data"]["dependencies"]["requires"]

-         if "references" in yml["data"]:

+         if "references" in yml["data"] and yml["data"]["references"]:

              if "community" in yml["data"]["references"]:

                  self.community = yml["data"]["references"]["community"]

              if "documentation" in yml["data"]["references"]:
@@ -125,7 +134,8 @@ 

                  self.tracker = yml["data"]["references"]["tracker"]

          if "xmd" in yml["data"]:

              self.xmd = yml["data"]["xmd"]

-         if "profiles" in yml["data"]:

+         if ("profiles" in yml["data"]

+                 and isinstance(yml["data"]["profiles"], dict)):

              for profile in yml["data"]["profiles"].keys():

                  self.profiles[profile] = ModuleProfile()

                  if "description" in yml["data"]["profiles"][profile]:
@@ -134,7 +144,8 @@ 

                  if "rpms" in yml["data"]["profiles"][profile]:

                      self.profiles[profile].rpms = \

                          set(yml["data"]["profiles"][profile]["rpms"])

-         if "components" in yml["data"]:

+         if ("components" in yml["data"]

+                 and isinstance(yml["data"]["components"], dict)):

              self.components = ModuleComponents()

              if "rpms" in yml["data"]["components"]:

                  self.components.rpms = ModuleRPMs()
@@ -215,12 +226,12 @@ 

              data["data"]["profiles"] = dict()

              for profile in self.profiles.keys():

                  if self.profiles[profile].description:

-                     if not profile in data["data"]["profiles"]:

+                     if profile not in data["data"]["profiles"]:

                          data["data"]["profiles"][profile] = dict()

                      data["data"]["profiles"][profile]["description"] = \

                          str(self.profiles[profile].description)

                  if self.profiles[profile].rpms:

-                     if not profile in data["data"]["profiles"]:

+                     if profile not in data["data"]["profiles"]:

                          data["data"]["profiles"][profile] = dict()

                      data["data"]["profiles"][profile]["rpms"] = \

                          list(self.profiles[profile].rpms)
@@ -386,7 +397,7 @@ 

          if self.components:

              if self.components.rpms:

                  for p, e in self.components.rpms.packages.items():

-                     if not "rationale" in e:

+                     if "rationale" not in e:

                          raise ValueError(p, "has no rationale")

          # TODO: Validate dependency version formats

          return True
@@ -528,7 +539,10 @@ 

      def requires(self, d):

          if d and not isinstance(d, dict):

              raise TypeError("Incorrect data type passed for requires")

-         self._requires = { str(k) : str(v) for k, v in d.items() }

+         if d:

+             self._requires = { str(k) : str(v) for k, v in d.items() }

+         else:

+             self._requires = dict()

  

      def add_requires(self, n, v):

          """Adds a required module dependency.

@@ -0,0 +1,311 @@ 

+ #/usr/bin/python3

+ # -*- coding: utf-8 -*-

+ 

+ 

+ # Copyright (c) 2016  Red Hat, Inc.

+ #

+ # Permission is hereby granted, free of charge, to any person obtaining a copy

+ # of this software and associated documentation files (the "Software"), to deal

+ # in the Software without restriction, including without limitation the rights

+ # to use, copy, modify, merge, publish, distribute, sublicense, and/or sell

+ # copies of the Software, and to permit persons to whom the Software is

+ # furnished to do so, subject to the following conditions:

+ #

+ # The above copyright notice and this permission notice shall be included in all

+ # copies or substantial portions of the Software.

+ #

+ # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR

+ # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,

+ # FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE

+ # AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER

+ # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,

+ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE

+ # SOFTWARE.

+ #

+ # Written by Jan Kaluza <jkaluza@redhat.com>

+ 

+ import unittest

+ 

+ import os

+ import sys

+ 

+ DIR = os.path.dirname(__file__)

+ sys.path.insert(0, os.path.join(DIR, ".."))

+ 

+ import modulemd

+ from yaml.scanner import ScannerError

+ 

+ class TestIO(unittest.TestCase):

+     def test_invalid_yaml(self):

+         document = """

+             document: modulemd

+             version: 0

+             data

+         """

+         mmd = modulemd.ModuleMetadata()

+         # Python yaml module raises wrong error message with "found" string

+         # instead of "find". We are testing for both variants here.

+         self.assertRaisesRegexp(ScannerError,

+                                 r"could not f(?:ou|i)nd expected ':'",

+                                 mmd.loads, document)

+ 

+     def test_object_value(self, yaml=None, value=""):

+         """

+         Replaces $VALUE in the the `yaml` input with the value provided

+         in the `value` variable and loads the yaml using modulemd library.

+         """

+         if not yaml:

+             return

+ 

+         yaml = yaml.replace("$VALUE", value)

+         mmd = modulemd.ModuleMetadata()

+         mmd.loads(yaml)

+         mmd.validate()

+ 

+     def test_object_missing(self, yaml=None):

+         """

+         Removes the line with the $VALUE from the yaml input and

+         loads the yaml using modulemd library.

+         """

+         if not yaml:

+             return

+ 

+         yaml = "\n".join(n for n in yaml.split("\n") if "$VALUE" not in n)

+         mmd = modulemd.ModuleMetadata()

+         mmd.loads(yaml)

+         mmd.validate()

+ 

+     def test_document(self):

+         document = """

+             document: $VALUE

+             version: 0

+             data:

+                 name: test

+                 version: 1.23

+                 release: 4

+                 summary: A test module

+                 description: >

+                     This module is a part of the modulemd test suite.

+                 license:

+                     module: [ MIT ]

+                     content: [ GPL+, GPLv3 ]

+         """

+         self.assertRaisesRegexp(ValueError,

+                                 "The supplied data isn't a valid modulemd document",

+                                 self.test_object_missing, document)

+         for value in ["", "modulemd2", "[]", "{}"]:

+             self.assertRaisesRegexp(ValueError,

+                                     "The supplied data isn't a valid modulemd document",

+                                     self.test_object_value, document, value)

+ 

+     def test_version(self):

+         document = """

+             document: modulemd

+             version: $VALUE

+             data:

+                 name: test

+                 version: 1.23

+                 release: 4

+                 summary: A test module

+                 description: >

+                     This module is a part of the modulemd test suite.

+                 license:

+                     module: [ MIT ]

+                     content: [ GPL+, GPLv3 ]

+         """

+         self.assertRaisesRegexp(ValueError, ".* is required",

+                                 self.test_object_missing, document)

+         for value in ["", "unknown", "[]", "{}", "9999"]:

+             self.assertRaisesRegexp(ValueError,

+                                     "The supplied metadata version isn't supported",

+                                     self.test_object_value, document, value)

+ 

+     def test_data(self):

+         document = """

+             document: modulemd

+             version: 0

+             data: $VALUE

+         """

+         self.assertRaisesRegexp(ValueError, ".* is required",

+                                 self.test_object_missing, document)

+         for value in ["", "unknown", "[]", "9999"]:

+             self.assertRaisesRegexp(ValueError,

+                                     ".* is required",

+                                     self.test_object_value, document, value)

+ 

+     def test_name(self):

+         document = """

+             document: modulemd

+             version: 0

+             data:

+                 name: $VALUE

+                 version: 1.23

+                 release: 4

+                 summary: A test module

+                 description: >

+                     This module is a part of the modulemd test suite.

+                 license:

+                     module: [ MIT ]

+                     content: [ GPL+, GPLv3 ]

+         """

+         self.assertRaisesRegexp(ValueError,

+                                 "name is required",

+                                 self.test_object_missing, document)

+         for value in ["", "test", "[]", "{}", "1"]:

+             self.test_object_value(document, value)

+ 

+     def test_version(self):

+         document = """

+             document: modulemd

+             version: 0

+             data:

+                 name: test

+                 version: $VALUE

+                 release: 4

+                 summary: A test module

+                 description: >

+                     This module is a part of the modulemd test suite.

+                 license:

+                     module: [ MIT ]

+                     content: [ GPL+, GPLv3 ]

+         """

+         self.assertRaisesRegexp(ValueError,

+                                 "version is required",

+                                 self.test_object_missing, document)

+         for value in ["", "test", "[]", "{}", "1"]:

+             self.test_object_value(document, value)

+ 

+     def test_release(self):

+         document = """

+             document: modulemd

+             version: 0

+             data:

+                 name: test

+                 version: 1.23

+                 release: $VALUE

+                 summary: A test module

+                 description: >

+                     This module is a part of the modulemd test suite.

+                 license:

+                     module: [ MIT ]

+                     content: [ GPL+, GPLv3 ]

+         """

+         self.assertRaisesRegexp(ValueError,

+                                 "release is required",

+                                 self.test_object_missing, document)

+         for value in ["", "test", "[]", "{}", "1"]:

+             self.test_object_value(document, value)

+ 

+     def test_summary(self):

+         document = """

+             document: modulemd

+             version: 0

+             data:

+                 name: test

+                 version: 1.23

+                 release: 4

+                 summary: $VALUE

+                 description: >

+                     This module is a part of the modulemd test suite.

+                 license:

+                     module: [ MIT ]

+                     content: [ GPL+, GPLv3 ]

+         """

+         self.assertRaisesRegexp(ValueError,

+                                 "summary is required",

+                                 self.test_object_missing, document)

+         for value in ["", "test", "[]", "{}", "1"]:

+             self.test_object_value(document, value)

+ 

+     def test_description(self):

+         document = """

+             document: modulemd

+             version: 0

+             data:

+                 name: test

+                 version: 1.23

+                 release: 4

+                 summary: A test module

+                 description: $VALUE

+                 license:

+                     module: [ MIT ]

+                     content: [ GPL+, GPLv3 ]

+         """

+         self.assertRaisesRegexp(ValueError,

+                                 "description is required",

+                                 self.test_object_missing, document)

+         for value in ["", "test", "[]", "{}", "1"]:

+             self.test_object_value(document, value)

+ 

+     def test_license(self):

+         document = """

+             document: modulemd

+             version: 0

+             data:

+                 name: test

+                 version: 1.23

+                 release: 4

+                 summary: A test module

+                 description: $VALUE

+                 license: $VALUE

+         """

+         self.assertRaisesRegexp(ValueError,

+                                 "description is required",

+                                 self.test_object_missing, document)

+ 

+         values = ["", "test", "1", "[]", "{}"]

+         values += ["{content:[MIT, GPL]}", "{module: []}"]

+         values += ["{module: }", "{module: {}}"]

+ 

+         for value in values:

+             self.assertRaisesRegexp(ValueError,

+                                     "at least one module license is required",

+                                     self.test_object_value, document, value)

+ 

+         self.test_object_value(document, "{module: [MIT]}")

+         self.test_object_value(document, "{module: MIT}")

+ 

+     def test_dependencies(self):

+         document = """

+             document: modulemd

+             version: 0

+             data:

+                 name: test

+                 version: 1.23

+                 release: 4

+                 summary: A test module

+                 description: >

+                     This module is a part of the modulemd test suite.

+                 license:

+                     module: [ MIT ]

+                 dependencies: $VALUE

+         """

+         self.test_object_missing(document)

+         for value in ["", "test", "1", "[]", "{}"]:

+             self.test_object_value(document, value)

+ 

+     def test_dependencies_type(self):

+         document = """

+             document: modulemd

+             version: 0

+             data:

+                 name: test

+                 version: 1.23

+                 release: 4

+                 summary: A test module

+                 description: >

+                     This module is a part of the modulemd test suite.

+                 license:

+                     module: [ MIT ]

+                 dependencies:

+                     requires: $VALUE

+         """

+         self.test_object_value(document, "")

+         self.test_object_value(document, "[]")

+         self.test_object_value(document, "{}")

+         self.assertRaisesRegexp(TypeError, "Incorrect data type passed",

+             self.test_object_value, document, "[foo, bar]")

+         self.test_object_value(document, "{modulemd: 42-42}")

+ 

+ if __name__ == "__main__":

+     unittest.main()

This is initial part of testing bad input files in modulemd test-suite. It also contains some fixes found by the tests.

1 new commit added

  • Add comments for test_object_value and test_object_missing
7 years ago

I'll comment on code here because pagure isn't good with preserving inline comments when changes are pushed.

  • What about not document in ymldocument not in yml? It's easier, more natural to read.
  • If you break lines, please take care that the continued line doesn't end up on the same indentation level as the following block so it's immediately clear what belongs to the block (yeah, I need to clean that up in existing code). E.g. this...:
    if ("license" in yml["data"] and isinstance(yml["data"]["license"], dict)
        and "content" in yml["data"]["license"]):
        self.content_licenses = set(yml["data"]["license"]["content"])
    

    ...should rather look like that:

    if ("license" in yml["data"] and isinstance(yml["data"]["license"], dict)
            and "content" in yml["data"]["license"]):
        self.content_licenses = set(yml["data"]["license"]["content"])
    

Hang on, not through the code yet -- slip of the finger (clicked on "Update Issue" rather than "Preview").

TestIO:

  • test_yaml(): I find creating document only for the default value of yaml is superfluous and forces one to read back on the following if not yaml: block. Rather put this inside the block and get rid of the additional variable?
  • FIx tense: "could not found expected ':'""could not find expected ':'"—may need fixing the place where the exception is raised, too.
  • Don't put whitespace around = for keyword argument defaults/values (PEP8). E.g.:

    def test_object_value(self, yaml = None, value = ""):
    

    should be:

    def test_object_value(self, yaml=None, value=""):
    
  • You can use a generator rather than a list comprehension if you use it only once, likewise forgo using str.find() if you only want to check for the existence of a substring here:

    yaml = "\n".join([n for n in yaml.split("\n") if n.find("$VALUE") == -1])
    

    yaml = "\n".join(n for n in yaml.split("\n") if "$VALUE" not in n)
    
  • Many methods create a ModuleMetadata object mmd without ever using it.

  • test_{version,release,summary,description}() and others have several similar calls to test_object_value() at the end, I like the loop you do in e.g. test_dependencies() better.

1 new commit added

  • Fix various style issues found during the review.
7 years ago

I think I've addressed the issues you have found except the "could not found expected" error message. This message is generated by the Python yaml module and therefore cannot be fixed in our code.

Thanks!

I think I've addressed the issues you have found except the "could not found expected" error message. This message is generated by the Python yaml module and therefore cannot be fixed in our code.

Ugh. Perhaps add a comment there? Otherwise someone coming across it might want to "fix" it. You know what, make the regex r"could not f(?:ou|i)nd expected ':'" plus the comment, then we're prepared for if the issue is fixed in the yaml module.

I like throwing away the extra functionality in test_invalid_yaml which is never used anyway. :wink:

Besides that, rather than fixing stuff in a separate commit, please fix them in the commit where they're introduced (git rebase -i is your friend there, if not, poke me). We're grooming the PR, history is not yet sacrosanct. :smiley:

1 new commit added

  • Test for both variants of Python yaml module error message - 'find' and also 'found'
7 years ago

rebased

7 years ago

rebased

7 years ago

rebased

7 years ago

Pull-Request has been closed by nphilipp

7 years ago