GreenScheduler / cats

CATS: the Climate-Aware Task Scheduler :cat2: :tiger2: :leopard:
https://greenscheduler.github.io/cats/
MIT License
50 stars 8 forks source link

Test failures #83

Closed andreww closed 5 months ago

andreww commented 5 months ago

Currently we have three failing tests on main. Two of them are easy to fix (see #82).

The test that still fails comes from line 218 of __init__.py where we expect four values to be returned by get_runtime_config (in configure.py) but that function returns six values. I think we just need to plumb in jobinfo and PU to __init__ but I'm not totally sure which way around the fix should go. Looks like some merge conflict resolution gone wrong to me. I think this is the cause of the error reported in #81.

While we're at it, I think the type hints in get_runtime_config are out of sync with the values that actually get returned (four rather than six types listed). It's not the only issue that mypy finds:

 cats/check_clean_arguments.py:31: error: Need type annotation for "info" (hint: "info: Dict[<type>, <type>] = ...")  [var-annotated]
cats/check_clean_arguments.py:31: error: Argument 1 to "dict" has incompatible type "list[tuple[str | Any, ...]]"; expected "Iterable[tuple[Never, Never]]"  [arg-type]
cats/check_clean_arguments.py:56: error: Expected keyword arguments, {...}, or dict(...) in TypedDict constructor  [misc]
cats/configure.py:50: error: Module has no attribute "eror"; maybe "error"?  [attr-defined]
cats/configure.py:67: error: Incompatible return value type (got "tuple[Mapping[str, Any], APIInterface, str, int, list[tuple[int, float]] | None, Any]", expected "tuple[dict[Any, Any], APIInterface, str, int]")  [return-value]
cats/configure.py:87: error: Missing return statement  [return]
cats/__init__.py:199: error: Incompatible types in assignment (expression has type "bytes", variable has type "CATSOutput")  [assignment]
cats/__init__.py:209: error: Missing return statement  [return]
Found 8 errors in 3 files (checked 10 source files)

Worth adding a mypy test to our CI? Worth setting github up to preclude merging / pushing onto main when the tests don't pass?

abhidg commented 5 months ago

@andreww both of those are good ideas!

abhidg commented 5 months ago

Possibly worth making the return type of get_runtime_config a TypedDict while at it - large tuples can be unwieldy.

andreww commented 5 months ago

@andreww both of those are good ideas!

In #82 I have added a mypy check step to the github actions and I have (just now) turned on the branch protection rule that mandates that the checks (linter, type checking, and test cases) pass before merging is allowed onto main. We can still bypass the branch protection rules (just like we could to avoid review) but this ought to stop main braking again. PRs will need to be "up to date" against main (so some rebasing / merging main into the feature branch may be needed). We can tweak this as needed.

That PR also fixes the "easy" mypy issues. Typing issues remain in two places. One is basically the same issue that causes the test case to fail. The other is validate_jobinfo - mypy doesn't like the way we pull the string apart. Probably easy to fix.

andreww commented 5 months ago

All dealt with by merge yesterday