Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: increase unit test coverage #52

Merged
merged 3 commits into from
Jan 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,12 @@ jobs:
run: pip install .

- name: Lint with Flake8
run: flake8 .
run: flake8 --max-line-length=120 .

- name: Run unit tests
run: coverage run --source tes -m pytest -W ignore::DeprecationWarning
run: |
pytest \
--cov=tes/ \
--cov-branch \
--cov-report=term-missing \
--cov-fail-under=99
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

6 changes: 3 additions & 3 deletions tes/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -193,14 +193,14 @@ def check_success(data: Task) -> bool:
time.sleep(0.5)

def _request_params(
self, data: Optional[str] = None,
params: Optional[Dict] = None
self, data: Optional[str] = None, params: Optional[Dict] = None
) -> Dict[str, Any]:
kwargs: Dict[str, Any] = {}
kwargs['timeout'] = self.timeout
kwargs['headers'] = {}
kwargs['headers']['Content-type'] = 'application/json'
kwargs['auth'] = (self.user, self.password)
if self.user is not None and self.password is not None:
kwargs['auth'] = (self.user, self.password)
Comment on lines +202 to +203
Copy link
Collaborator Author

@uniqueg uniqueg Feb 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if data:
kwargs['data'] = data
if params:
Expand Down
32 changes: 14 additions & 18 deletions tes/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,15 @@
from typing import Any, Dict, List, Optional, Tuple, Type, Union


@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,
Comment on lines -24 to -25
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

f"'{attr.name}' must be a list of {self.type!r} (got "
f"{value!r}", attr
Comment on lines -13 to +21
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)

def __repr__(self) -> str:
Expand Down Expand Up @@ -60,15 +56,15 @@ def strconv(value: Any) -> Any:
# since an int64 value is encoded as a string in json we need to handle
# conversion
def int64conv(value: Optional[str]) -> Optional[int]:
if value is not None:
return int(value)
return value
if value is None:
return value
return int(value)
Comment on lines -63 to +61
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



def timestampconv(value: Optional[str]) -> Optional[datetime]:
if value is not None:
return dateutil.parser.parse(value)
return value
if value is None:
return value
return dateutil.parser.parse(value)


def datetime_json_handler(x: Any) -> str:
Expand Down Expand Up @@ -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:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

errs.append("Executor command must be provided")
if e.stdin is not None:
if not os.path.isabs(e.stdin):
Expand All @@ -306,8 +302,8 @@ def is_valid(self) -> Tuple[bool, Union[None, TypeError]]:
if not os.path.isabs(e.stderr):
errs.append("Executor stderr must be an absolute path")
if e.env is not None:
for k, v in e.env:
if not isinstance(k, str) and not isinstance(k, str):
Comment on lines -309 to -310
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for k, v in e.env.items():
if not isinstance(k, str) and not isinstance(v, str):
errs.append(
"Executor env keys and values must be StrType"
)
Expand Down Expand Up @@ -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:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python3 requires .items(), as above.

for k, v in self.tags.items():
if not isinstance(k, str) and not isinstance(k, str):
errs.append(
"Tag keys and values must be StrType"
Expand Down
26 changes: 12 additions & 14 deletions tes/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,20 @@ def __init__(self, *args, **kwargs):


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@uniqueg uniqueg Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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")
Comment on lines +30 to +43
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


d: Dict[str, Any] = {}
if convert_camel_case:
Expand Down Expand Up @@ -77,16 +83,8 @@ def _unmarshal(v: Any, obj: Type) -> Any:
field = v
omap = fullOmap.get(o.__name__, {})
if k in omap:
if isinstance(omap[k], tuple):
try:
obj = omap[k][0]
field = _unmarshal(v, obj)
except Exception:
obj = omap[k][1]
field = _unmarshal(v, obj)
Comment on lines -80 to -86
Copy link
Collaborator Author

@uniqueg uniqueg Feb 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else:
obj = omap[k]
field = _unmarshal(v, obj)
obj = omap[k]
field = _unmarshal(v, obj)
r[k] = field

try:
Expand Down
Empty file added tests/__init__.py
Empty file.
1 change: 1 addition & 0 deletions tests/requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@ coverage>=6.5.0
coveralls>=3.3.1
flake8>=5.0.4
pytest>=7.2.1
pytest-cov>=4.0.0
requests_mock>=1.10.0
Loading