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 failures #83

Closed
andreww opened this issue Apr 8, 2024 · 4 comments
Closed

Test failures #83

andreww opened this issue Apr 8, 2024 · 4 comments

Comments

@andreww
Copy link
Collaborator

andreww commented Apr 8, 2024

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?

@andreww andreww mentioned this issue Apr 8, 2024
@abhidg
Copy link
Contributor

abhidg commented Apr 8, 2024

@andreww both of those are good ideas!

@abhidg
Copy link
Contributor

abhidg commented Apr 8, 2024

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

@andreww
Copy link
Collaborator Author

andreww commented Apr 9, 2024

@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
Copy link
Collaborator Author

andreww commented May 1, 2024

All dealt with by merge yesterday

@andreww andreww closed this as completed May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants