-
Notifications
You must be signed in to change notification settings - Fork 9
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
Comments
@andreww both of those are good ideas! |
Possibly worth making the return type of |
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. |
All dealt with by merge yesterday |
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 byget_runtime_config
(inconfigure.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:Worth adding a mypy test to our CI? Worth setting github up to preclude merging / pushing onto main when the tests don't pass?
The text was updated successfully, but these errors were encountered: