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

Fix test failures on main #82

Closed
wants to merge 8 commits into from

Conversation

andreww
Copy link
Collaborator

@andreww andreww commented Apr 8, 2024

This fixes two (of the three) failing tests on main.

The issues that are fixed involves a test case where newlines are printed by cats, which are not in the reference output in the tests (an easy fix).

Out test cases were falling over when cats printed
newlines which were unexpected in the test cases.
This puts the newlines in the expected strings in
the tests.
@andreww andreww mentioned this pull request Apr 8, 2024
@andreww andreww force-pushed the fix_the_tests branch 2 times, most recently from bee9ee8 to 407d0b7 Compare April 9, 2024 08:19
eror -> error, mypy picks this up
Always return an integer from main (so we sys.exit
0 if everything works). Don't reuse output for the
subrocess output (bytes) and the cats output type
that goes into the subprocess.
.github/workflows/tests.yml Outdated Show resolved Hide resolved
The return type now matches what we return, but could probably use
some cleanup. We don't make use of the returned _jobinfo and _PUE
but we probably should. We pull out jobinfo below in main. We
probably shouldn't.
But I cannot help but think that this is a long way
around. Unwrapping the dictonary construtor so that
mypy can see the types (aroud line 32) seems more
complex than it should be, and the cast at the end
seems painful. Can we not just build the TypedDict
as we go?
@andreww
Copy link
Collaborator Author

andreww commented Apr 9, 2024

I think some of my "fixes" in here are properly addressed in #80, 058a780 in particular. @tlestang - we should probably figure out how to order these two PRs, or if it's best to chop this one up / just take the stuff that's not involved in #80.

This will help us keep track of current test coverage
@andreww andreww mentioned this pull request Apr 10, 2024
@tlestang
Copy link
Collaborator

Hi @andreww - I split the rewriting/testing of the carbon footprint estimation module into two PR #79 and #80. I just merged #80 in which fixed the failing test on main. I also resolved a few inconsistencies (unused function or variable like jobinfo) that were necessary to introduce in order to integrate the changes in two distinct steps without breaking cats.

@@ -24,7 +24,8 @@
__all__ = ["get_runtime_config"]


def get_runtime_config(args) -> tuple[dict, APIInterface, str, int]:
def get_runtime_config(args) -> tuple[Mapping[str, Any], APIInterface, str, int,
Union[list[tuple[int, float]],None],Any]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

one of these cases where typing really isn't worth the trouble in my opinion.

@tlestang
Copy link
Collaborator

About the conflicts:

  • The check_clean_arguments.py was removed, because the job of processing and validating command line arguments is now carried out in the config module.
  • After Reduce carbon footprint estimation to total energy amount #80 the in-memory representation of the configuration file (config dictionary) doesn't have to leave the config module, so the config.get_runtime_config function doesn't have to return it in __init__.py. Same goes for _jobinfo.

@andreww
Copy link
Collaborator Author

andreww commented Apr 12, 2024

Thanks - I'll reorder and rebase this branch so that it makes sense now #80 is merged.

@andreww
Copy link
Collaborator Author

andreww commented Apr 17, 2024

Rebased and cleaned all of this up. Two new PRs inbound. We can kill this one.

@andreww andreww closed this Apr 17, 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

Successfully merging this pull request may close these issues.

3 participants