test: increase unit test coverage #52

uniqueg · 2023-02-12T14:17:22Z

Increase unit test coverage both from the point of view of covering more reaslistic scenarios and from the point of view of covering more code statements
Code coverage now at >99%, with the only missing statements in an unused function that may be removed (see Method "HTTPClient.wait()" defined but unused #48 and chore: remove unused method & exception #50)
Replace unittest's deprecated .assertEquals() and .assertAlmostEquals() calls with .assertEqual() and .assertAlmostEqual
Do not add auth as parameter to requests if user and/or password are not specified (accepting None for these is deprecated from requests 3.0.0)
Remove several code smells/minor bugs along the process (see comments in proposed code changes)
Replace test call in CI workflow to use pytest-cov (now in tests/requirements.txt), along with the branch testing flag --cov-branch switched on and the --cov-fail-under option set to 99 (can be changed to 100, once chore: remove unused method & exception #50 is merged or tests are provided for the missing statement in tes.client.HTTPClient.wait())

Resolves #47

uniqueg · 2023-02-12T14:41:57Z

tes/utils.py

-            if isinstance(omap[k], tuple):
-                try:
-                    obj = omap[k][0]
-                    field = _unmarshal(v, obj)
-                except Exception:
-                    obj = omap[k][1]
-                    field = _unmarshal(v, obj)


I have unmarshalled to every TES object and this code never ran. Also the model definitions do not contain any tuples. Maybe this was added to support some legacy model, but I'm pretty sure the code is not used with the current models.

Anyway, instead of trying to create some artificial class to use with the unmarshal() function to force this code to run, I have just removed it. We can always add more code if needed by future model versions.

Or maybe I missed something? Do you know why these tuple conditionals made it here in the first place?

My guess is that the tuple conditionals were added when outputs and logs were represented as tuples in a much earlier TES schema (first added here in commit 8f43a45):

py-tes/tes/utils.py

Lines 35 to 43 in 8f43a45

omap = {

"tasks": Task,

"inputs": TaskParameter,

"outputs": (TaskParameter, OutputFileLog),

"logs": (TaskLog, ExecutorLog),

"ports": Ports,

"resources": Resources,

"executors": Executor

}

Agreed on the removal as it's no longer necessary.

uniqueg · 2023-02-12T14:47:52Z

tes/utils.py

+    m: Any = None
    if isinstance(j, str):
-        m = json.loads(j)
-    elif isinstance(j, dict):
-        m = j
+        try:
+            m = json.loads(j)
+        except json.decoder.JSONDecodeError:
+            pass
    elif j is None:
        return None
    else:
-        raise TypeError("j must be a str, a dict or None")
+        m = j
+
+    if not isinstance(m, dict):
+        raise TypeError("j must be a dictionary, a JSON string evaluation to "
+                        "a dictionary, or None")


Previously the behavior of unmarshal() was not well defined for strings that could be parsed to JSON but that do not end up being dictionaries (list, int, float, bool, None). And for those strings that could not be parsed, a JSON decode error would be implicitly raised by the code, which I thought was a little inconsistent.

So here I am ignoring decode parsers but then check that we are really dealing with a dictionary later on, and if not, give a single specific message of what input is expected here (dict, JSON string evaluating to dict, or None). I find it a bit easier to read as well.

uniqueg · 2023-02-12T14:52:37Z

tes/models.py

-                    for k, v in e.env:
-                        if not isinstance(k, str) and not isinstance(k, str):


Obviously the check should be for both the key and the value. And Python3 requires .items().

uniqueg · 2023-02-12T14:52:56Z

tes/models.py

@@ -339,7 +335,7 @@ def is_valid(self) -> Tuple[bool, Union[None, TypeError]]:
                        errs.append("Volume paths must be absolute")

        if self.tags is not None:
-            for k, v in self.tags:


Python3 requires .items(), as above.

uniqueg · 2023-02-12T14:55:47Z

tes/models.py

@@ -294,7 +290,7 @@ def is_valid(self) -> Tuple[bool, Union[None, TypeError]]:
            for e in self.executors:
                if e.image is None:
                    errs.append("Executor image must be provided")
-                if len(e.command) == 0:
+                if e.command is None or len(e.command) == 0:


I think this additional check is necessary, or otherwise an error with traceback is thrown when the user/client passes command=None - which is rather inconsistent, as we otherwise nicely collect all the (common) problems that may arise.

uniqueg · 2023-02-12T14:57:31Z

tes/models.py

-    if value is not None:
-        return int(value)
-    return value
+    if value is None:
+        return value
+    return int(value)


No functional changes here. This one (and the next right below for timestampconv() I just reordered for improved readability (at least in my opinion)

uniqueg · 2023-02-12T15:05:07Z

tes/models.py

-@attrs
+@attrs(repr=False)
 class _ListOfValidator(object):
    type: Type = attrib()

-    def __call__(self, inst, attr, value):
-        """
-        We use a callable class to be able to change the ``__repr__``.
-        """
+    def __call__(self, inst, attr, value) -> None:
        if not all([isinstance(n, self.type) for n in value]):
            raise TypeError(
-                "'{attr.name}' must be a list of {self.type!r} (got {value!r} "
-                "that is a list of {values[0].__class__!r}).",
-                attr, self.type, value,
+                f"'{attr.name}' must be a list of {self.type!r} (got "
+                f"{value!r}", attr


This one took me a while: I couldn't figure out why the __repr__ never got called in the unit tests and instead repr() on the validator always returned _ListOfValidator(type=<class 'tes.models.Input'>) or some such. Turns out that attrs overrides __repr__ unless you explicitly tell it not to. Probably due to a change in attrs that came up now that we upgraded dependencies. Setting repr to False in the decorator fixed that.

uniqueg · 2023-02-12T15:07:05Z

tes/models.py

-                "that is a list of {values[0].__class__!r}).",
-                attr, self.type, value,


Checking for just values[0] may be misleading if a list is composed of several different types. I think it's fine to just print the value as received, the user can then see themselves what they passed.

uniqueg · 2023-02-12T15:08:18Z

tes/client.py

+        if self.user is not None and self.password is not None:
+            kwargs['auth'] = (self.user, self.password)


This is to address a depreciation warning as already mentioned in the PR description.

lbeckman314 · 2023-02-15T17:53:51Z

.github/workflows/tests.yml

+            --cov=tes/ \
+            --cov-branch \
+            --cov-report=term-missing \
+            --cov-fail-under=99


The --cov-fail-under=99 condition is a great addition. Awesome to see and something I'll use from now on in other pytest projects!

Yeah, I've also only found out about it only now, when I replaced the old test runner nose (which had an equivalent option that was used in the previous CI version) with pytest. Note that it requires the pytest-cov extension, though.

lbeckman314 · 2023-02-15T18:57:02Z

tes/utils.py

@@ -27,14 +27,20 @@ def __init__(self, *args, **kwargs):


 def unmarshal(j: Any, o: Type, convert_camel_case=True) -> Any:


I'm reviewing the Python docs now, but what are the use cases for a variable having type Type vs Any like o and j have here?

I admit that I have added the type hints in one of the last PRs rather quickly, mostly aiming for completion and making the VS Code Pyright extension shut up (and possibly to make it easier to include static code analysis via mypy in the CI in the future, if desired). And sometimes I have resorted to annotating with Any, which is indeed not extremely helpful.

Type on the other hand is probably a reasonable type hint for o though, as it is the appropriate (generic) type hint for a class. It may be more accurate to annotate it as the union of all classes that the function is supposed to handle, but it would end up being really really long.

So to answer your question: If I called the function with an object that is not a class, but, e.g., an instance of a class, tools like mypy or the VS Code Pyright extension would complain. For example, unmarshal(j=..., o=str) is fine, but unmarshal(j=..., o="some_string") is not, because:

>>> type(str) <class 'type'> >>> type("some_string") <class 'str'>

That's really interesting! I haven't encountered the Type type before but that makes perfect sense.

lbeckman314 · 2023-02-15T19:12:14Z

tes/utils.py

-            if isinstance(omap[k], tuple):
-                try:
-                    obj = omap[k][0]
-                    field = _unmarshal(v, obj)
-                except Exception:
-                    obj = omap[k][1]
-                    field = _unmarshal(v, obj)


My guess is that the tuple conditionals were added when outputs and logs were represented as tuples in a much earlier TES schema (first added here in commit 8f43a45):

py-tes/tes/utils.py

Lines 35 to 43 in 8f43a45

omap = {

"tasks": Task,

"inputs": TaskParameter,

"outputs": (TaskParameter, OutputFileLog),

"logs": (TaskLog, ExecutorLog),

"ports": Ports,

"resources": Resources,

"executors": Executor

}

Agreed on the removal as it's no longer necessary.

lbeckman314

LGTM! All tests including the new ones pass on my machine and the changes are extremely well thought out.

uniqueg requested a review from kellrott February 12, 2023 14:17

uniqueg force-pushed the unit-test-coverage branch from 93dc6ab to e12a97d Compare February 12, 2023 14:18

uniqueg changed the base branch from master to release-0.5.0 February 12, 2023 14:20

uniqueg force-pushed the unit-test-coverage branch 4 times, most recently from de0afc7 to 1c99f8d Compare February 12, 2023 14:35

uniqueg commented Feb 12, 2023

View reviewed changes

uniqueg force-pushed the unit-test-coverage branch from 1c99f8d to 3283e6a Compare February 12, 2023 15:03

uniqueg commented Feb 12, 2023

View reviewed changes

uniqueg force-pushed the unit-test-coverage branch from 3283e6a to e5647ca Compare February 12, 2023 15:14

test: increase unit test coverage

756d3c4

uniqueg force-pushed the unit-test-coverage branch from e5647ca to 756d3c4 Compare February 12, 2023 15:18

kellrott requested a review from lbeckman314 February 15, 2023 17:43

lbeckman314 reviewed Feb 15, 2023

View reviewed changes

lbeckman314 approved these changes Feb 15, 2023

View reviewed changes

uniqueg mentioned this pull request Mar 1, 2023

Method "HTTPClient.wait()" defined but unused #48

Open

lbeckman314 added 2 commits January 8, 2024 15:32

Merge branch 'release-1.0.0' into unit-test-coverage

ef954d5

Update tests.yml

d030b35

lbeckman314 merged commit 93ad675 into release-1.0.0 Jan 8, 2024
5 checks passed

lbeckman314 deleted the unit-test-coverage branch February 16, 2024 03:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: increase unit test coverage #52

test: increase unit test coverage #52

uniqueg commented Feb 12, 2023 •

edited

Loading

uniqueg Feb 12, 2023 •

edited

Loading

lbeckman314 Feb 15, 2023

uniqueg Feb 12, 2023

uniqueg Feb 12, 2023

uniqueg Feb 12, 2023

uniqueg Feb 12, 2023

uniqueg Feb 12, 2023

uniqueg Feb 12, 2023

uniqueg Feb 12, 2023

uniqueg Feb 12, 2023 •

edited

Loading

lbeckman314 Feb 15, 2023

uniqueg Feb 16, 2023

lbeckman314 Feb 15, 2023

uniqueg Feb 16, 2023 •

edited

Loading

uniqueg Feb 16, 2023

lbeckman314 Feb 16, 2023

lbeckman314 Feb 15, 2023

lbeckman314 left a comment •

edited

Loading

	omap = {
	"tasks": Task,
	"inputs": TaskParameter,
	"outputs": (TaskParameter, OutputFileLog),
	"logs": (TaskLog, ExecutorLog),
	"ports": Ports,
	"resources": Resources,
	"executors": Executor
	}

		for k, v in e.env:
		if not isinstance(k, str) and not isinstance(k, str):

		"that is a list of {values[0].__class__!r}).",
		attr, self.type, value,

		if self.user is not None and self.password is not None:
		kwargs['auth'] = (self.user, self.password)

		@@ -27,14 +27,20 @@ def __init__(self, args, *kwargs):


		def unmarshal(j: Any, o: Type, convert_camel_case=True) -> Any:

test: increase unit test coverage #52

test: increase unit test coverage #52

Conversation

uniqueg commented Feb 12, 2023 • edited Loading

uniqueg Feb 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uniqueg Feb 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uniqueg Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbeckman314 left a comment • edited Loading

Choose a reason for hiding this comment

uniqueg commented Feb 12, 2023 •

edited

Loading

uniqueg Feb 12, 2023 •

edited

Loading

uniqueg Feb 12, 2023 •

edited

Loading

uniqueg Feb 16, 2023 •

edited

Loading

lbeckman314 left a comment •

edited

Loading