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 CI test failures in feature/capgen, address pylint warnings #412

Conversation

climbfuji
Copy link
Collaborator

@climbfuji climbfuji commented Nov 2, 2021

Description

This PR fixes the CI test failures reported in #411.

It also bumps up the pylint rating for test/unit_tests/test_metadata_table.py from below 9 to 10/10 (yay) by removing the unused variable tables in many places.

Caveats

When running

PYTHONPATH=$(pwd)/scripts:$(pwd)/scripts/parse_tools pytest

locally (this is what the GitHub workflow/CI test does), I still get several warnings:

dom.heinzeller@heinzeller-lt:~/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102 [gccgfortran-py3-hpc-stack-1.2.0]>  PYTHONPATH=$(pwd)/scripts:$(pwd)/scripts/parse_tools pytest
....                                                                                                                                                                                                                                               [100%]
==================================================================================================================== warnings summary ====================================================================================================================
scripts/state_machine.py:165
  /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102/scripts/state_machine.py:165: DeprecationWarning: Flags not at the start of the expression '(?:(?i)timestep_init' (truncated)
    regex = re.compile(value[2] + r"$")

scripts/state_machine.py:166
scripts/state_machine.py:166
scripts/state_machine.py:166
scripts/state_machine.py:166
scripts/state_machine.py:166
  /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102/scripts/state_machine.py:166: DeprecationWarning: Flags not at the start of the expression '([A-Za-z][A-Za-z0-9_' (truncated)
    function = re.compile(FORTRAN_ID + r"_(" + value[2] + r")$")

scripts/state_machine.py:165
  /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102/scripts/state_machine.py:165: DeprecationWarning: Flags not at the start of the expression '(?:(?i)timestep_fina' (truncated)
    regex = re.compile(value[2] + r"$")

scripts/state_machine.py:165
  /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102/scripts/state_machine.py:165: DeprecationWarning: Flags not at the start of the expression '(?:(?i)init(?:ial(?:' (truncated)
    regex = re.compile(value[2] + r"$")

scripts/state_machine.py:165
  /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102/scripts/state_machine.py:165: DeprecationWarning: Flags not at the start of the expression '(?:(?i)final(?:ize)?' (truncated)
    regex = re.compile(value[2] + r"$")

scripts/state_machine.py:165
  /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102/scripts/state_machine.py:165: DeprecationWarning: Flags not at the start of the expression '(?:(?i)run)$'
    regex = re.compile(value[2] + r"$")

-- Docs: https://docs.pytest.org/en/latest/warnings.html
4 passed, 10 warnings in 0.08s

@climbfuji climbfuji marked this pull request as ready for review November 2, 2021 20:21
Copy link
Collaborator

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

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

Wondering why whitespace changes need to be in test_metadata_table.py

@@ -85,8 +85,7 @@ def test_bad_type_name(self):

#Exercise
with self.assertRaises(Exception) as context:
tables = parse_metadata_file(filename, known_ddts,
self._DUMMY_RUN_ENV)
parse_metadata_file(filename, known_ddts, self._DUMMY_RUN_ENV)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the point of all these whitespace changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, now the entire call fits in one line below 80 characters and thus easily on a terminal screen. I'll change it back ...

Copy link
Collaborator

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

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

The issue before was not the whitespace change but why you are choosing to add unrelated changes to a bugfix PR. To quote a developer I know:

I think we should try to be good stewards of GitHub code change practices and create smaller PRs that contain logically related changes but nothing else / not much else otherwise going forward.

test/unit_tests/test_metadata_table.py Outdated Show resolved Hide resolved
@climbfuji
Copy link
Collaborator Author

The issue before was not the whitespace change but why you are choosing to add unrelated changes to a bugfix PR. To quote a developer I know:

I think we should try to be good stewards of GitHub code change practices and create smaller PRs that contain logically related changes but nothing else / not much else otherwise going forward.

Except that there is a difference between removing a newline when changes that are bug fixes do shorten the lines so that they fit within reasonable limits or othewise require adjusting the indentation for the second line (and all this on 19 lines in a single file), and thousands of lines of code changes that are unrelated in one PR ...

At any rate, I committed your suggestion and changed all other occurrences to match it. Note that your suggested change also removed the newline (45415b5).

@gold2718
Copy link
Collaborator

gold2718 commented Nov 3, 2021

Except that there is a difference between removing a newline when changes that are bug fixes do shorten the lines so that they fit within reasonable limits ...

My issue is that unless we have a definition of "reasonable limits", we will keep having this problem.

Copy link
Collaborator

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

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

Still not sure why test_metadata_table.py needed any modification to fix the CI bug in test_metadata_parser.py

@climbfuji climbfuji merged commit 105d8b5 into NCAR:feature/capgen Nov 4, 2021
@climbfuji climbfuji deleted the fix_ci_test_failures_feature_capgen_20211102 branch June 27, 2022 03:03
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.

2 participants