Skip to content

Improve umdp3 checks I - preventing easy false positives #1

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.

Sign up for GitHub

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

Closed
r-sharp wants to merge 4 commits into from
Closed

Improve umdp3 checks I - preventing easy false positives #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6db154e
minor tweaks to CamelCase var checker, plus extra tests
r-sharp Apr 28, 2026
d14935c
Change order of assertions to improve error diagnosis when the tests …
r-sharp Apr 28, 2026
edcdd20
Improving checks on unseparated keywords
r-sharp Apr 29, 2026
f5364e9
preventing false failures on cpp directives, and adding those caases …
r-sharp Apr 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions script_umdp3_checker/tests/example_fortran_code.F90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,10 @@ SUBROUTINE example (xlen,ylen,l_unscale,input1,input2, &
INTEGER, INTENT(IN) :: xlen !Length of first dimension of the arrays.
INTEGER, INTENT(IN) :: ylen !Length of second dimension of the arrays.
LOGICAL, INTENT(IN) :: l_unscale ! switch scaling off.
REAL, INTENT(IN) :: input1(xlen, ylen) !First input array
REAL, INTENT(IN OUT) :: input2(xlen, ylen) !Second input array
REAL, INTENT(OUT) :: output(xlen, ylen) !Contains the result
REAL, INTENT(IN) :: input1(xlen, ylen) !First input array
REAL, INTENT(IN OUT) :: input2(xlen, ylen) ! INOUT Second input array
REAL, INTENT(OUT) :: output(xlen, ylen) !Contains the result
REAL, PARAMETER :: b_pollonator(4) = [ 0.0, 11.2, 6.6, 3.6]
LOGICAL, INTENT(IN), OPTIONAL :: l_loud_opt !optional debug flag
! Local variables
INTEGER(KIND=jpim), PARAMETER :: zhook_in = 0 ! DrHook tracing entry
Expand Down
4 changes: 4 additions & 0 deletions script_umdp3_checker/tests/test_fortran_checks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ def test_openmp_sentinels_in_column_one(lines, expected_result):
(["i=0", "i=i+1", "PRINT*,i"], 0, "No keywords"),
(["PROGRAM test", "i=0", "ENDIF"], 1, "One keyword unseparated"),
(["i=0", "ENDPARALLELDO", "END DO"], 1, "One keyword unseparated in middle"),
(["REAL, INTENT(IN OUT) :: lambda_pole !INOUT Longitude of pole",
"REAL, INTENT(OUT) :: Dave", "DO", "nothing", "END DO"], 0,
"unseparated keyword in comment"),
(["#endif", "i=i+1", "PRINT*,i"], 0, "Safely ignore cpp commands"),
]


Expand Down
130 changes: 93 additions & 37 deletions script_umdp3_checker/tests/test_umdp3_rules_S3.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,6 @@ def test_concatenate_lines(example_fortran_lines):
@pytest.mark.parametrize(
"changes_list, expected_passed, expected_failure_count, expected_errors",
[
# Valid: MODULE example_mod ... END MODULE example_mod (no changes)
([], True, 0, {}),
# Invalid: No program unit found (delete MODULE line)
(
[["replace", 12, ["! No module declaration here"]]],
Expand All @@ -162,7 +160,7 @@ def test_concatenate_lines(example_fortran_lines):
),
# Invalid: Mismatched END statement
(
[["replace", 120, ["END MODULE wrong_mod_name"]]],
[["replace", 121, ["END MODULE wrong_mod_name"]]],
False,
1,
{
Expand All @@ -173,7 +171,7 @@ def test_concatenate_lines(example_fortran_lines):
),
# Invalid: No END statement found
(
[["replace", 120, ["! Missing END MODULE"]]],
[["replace", 121, ["! Missing END MODULE"]]],
False,
1,
{
Expand All @@ -182,12 +180,22 @@ def test_concatenate_lines(example_fortran_lines):
]
},
),
# Pass ccp directives #if !defined(MCT)
(
[["replace", 1, ["#if !defined(MCT)"]], ["add", 121, ["#endif"]]],
True,
0,
{},
),
# Valid: MODULE example_mod ... END MODULE example_mod (no changes)
([], True, 0, {}),
],
ids=[
"Valid module with matching END",
"No program unit found",
"Mismatched END statement",
"No END statement",
"Valid checking cpp directives pass",
"Valid module with matching END",
],
)
def test_r3_1_1_there_can_be_only_one(
Expand All @@ -199,15 +207,15 @@ def test_r3_1_1_there_can_be_only_one(
):
modified_fortran_lines = modify_fortran_lines(example_fortran_lines, changes_list)
result = r3_1_1_there_can_be_only_one(modified_fortran_lines)
assert result.passed == expected_passed
assert result.failure_count == expected_failure_count
errors = result.errors
assert len(errors) == len(expected_errors)
for error, lines_list in errors.items():
assert error in expected_errors
assert len(lines_list) == len(expected_errors[error])
for line_no in lines_list:
assert line_no in expected_errors[error]
assert len(lines_list) == len(expected_errors[error])
assert len(errors) == len(expected_errors)
assert result.failure_count == expected_failure_count
assert result.passed == expected_passed


# =================================================================
Expand Down Expand Up @@ -238,14 +246,14 @@ def test_r3_2_1_check_crown_copyright(
modified_fortran_lines = modify_fortran_lines(example_fortran_lines, changes_list)
result = r3_2_1_check_crown_copyright(modified_fortran_lines)
failure_count = result.failure_count
assert failure_count == expected_result
errors = result.errors
assert len(errors) == len(expected_errors)
for error, lines_list in errors.items():
assert error in expected_errors
assert len(lines_list) == len(expected_errors[error])
for line_no in lines_list:
assert line_no in expected_errors[error]
assert len(lines_list) == len(expected_errors[error])
assert len(errors) == len(expected_errors)
assert failure_count == expected_result


# =================================================================
Expand All @@ -272,7 +280,6 @@ def test_r3_2_1_check_crown_copyright(
+ '":" // RoutineName, zhook_out, zhook_handle) ! extra comment'
],
],
["replace", 42, ["use yomhook, ONLY: lhook, dr_hook"]],
],
2,
{"line too long": [73, 117]},
Expand All @@ -288,14 +295,14 @@ def test_r3_3_2_line_too_long(
modified_fortran_lines = modify_fortran_lines(example_fortran_lines, changes_list)
result = r3_3_2_line_too_long(modified_fortran_lines)
failure_count = result.failure_count
assert failure_count == expected_result
errors = result.errors
assert len(errors) == len(expected_errors)
for error, lines_list in errors.items():
assert error in expected_errors
assert len(lines_list) == len(expected_errors[error])
for line_no in lines_list:
assert line_no in expected_errors[error]
assert len(lines_list) == len(expected_errors[error])
assert len(errors) == len(expected_errors)
assert failure_count == expected_result


# =================================================================
Expand All @@ -313,24 +320,34 @@ def test_r3_3_2_line_too_long(
3,
{"lowercase keyword: Module": [12], "lowercase keyword: use": [39, 42]},
),
(
[
["add", 44, ["#if defined(SOMETHING)"]],
["add", 47, ["#else", "INTEGER, INTENT(INOUT) :: xlen",
"INTEGER, INTENT(OUT) :: ylen",
"LOGICAL, INTENT(IN) :: l_WhoopSies = .FALSE.", "#endif"]],
],
0,
{},
),
([], 0, {}), # No changes, expect no errors
],
ids=["3 Lowercase Errors", "No Lowercase Errors"],
ids=["3 Lowercase Errors", "Pass cpp directives", "No Lowercase Errors"],
)
def test_r3_4_1_capitalised_keywords(
example_fortran_lines, changes_list, expected_result, expected_errors
):
modified_fortran_lines = modify_fortran_lines(example_fortran_lines, changes_list)
result = r3_4_1_capitalised_keywords(modified_fortran_lines)
failure_count = result.failure_count
assert failure_count == expected_result
errors = result.errors
assert len(errors) == len(expected_errors)
for error, lines_list in errors.items():
assert error in expected_errors
assert len(lines_list) == len(expected_errors[error])
for line_no in lines_list:
assert line_no in expected_errors[error]
assert len(lines_list) == len(expected_errors[error])
assert len(errors) == len(expected_errors)
assert failure_count == expected_result


# =================================================================
Expand All @@ -347,19 +364,19 @@ def test_r3_4_1_capitalised_keywords(
45,
["INTEGER, INTENT(IN) :: XLEN !Length of first dim of the arrays."],
],
["replace", 60, ["REAL :: var1, DAVE_2, HiPPo"]],
["replace", 61, ["REAL :: var1, DAVE_2, HiPPo"]],
],
2,
{
"Found UPPERCASE variable name in declaration at line 45: XLEN": [45],
"Found UPPERCASE variable name in declaration at line 60: DAVE_2": [60],
"Found UPPERCASE variable name in declaration at line 45: \"XLEN\"": [45],
"Found UPPERCASE variable name in declaration at line 61: \"DAVE_2\"": [61],
},
),
(
[
[
"add",
58,
59,
["LOGICAL :: l_whizz_bang = .FALSE. ! optimisation flag"],
],
],
Expand All @@ -370,7 +387,7 @@ def test_r3_4_1_capitalised_keywords(
[
[
"add",
60,
61,
[
"REAL :: VARIaBLE_1, variable_2, &",
" VARIABLE_3, Hot_Potato, Baked Potato &",
Expand All @@ -380,7 +397,7 @@ def test_r3_4_1_capitalised_keywords(
],
[
"replace",
56,
57,
[
"INTEGER :: j ! Loop counter &",
"INTEGER :: k ! Loop counter &",
Expand All @@ -398,25 +415,59 @@ def test_r3_4_1_capitalised_keywords(
],
5,
{
"Found UPPERCASE variable name in declaration at line 45: XLEN": [45],
"Found UPPERCASE variable name in declaration at line 58: IJ": [58],
"Found UPPERCASE variable name in declaration at line 62: CASPVAR": [
62
"Found UPPERCASE variable name in declaration at line 45: \"XLEN\"": [45],
"Found UPPERCASE variable name in declaration at line 59: \"IJ\"": [59],
"Found UPPERCASE variable name in declaration at line 63: \"CASPVAR\"": [
63
],
"Found UPPERCASE variable name in declaration at line 62: VARIABLE_3": [
62
"Found UPPERCASE variable name in declaration at line 63: \"VARIABLE_3\"": [
63
],
"Found UPPERCASE variable name in declaration at line 62: CAPS_VAR": [
62
"Found UPPERCASE variable name in declaration at line 63: \"CAPS_VAR\"": [
63
],
},
),
(
[
["delete", 48, None],
[
"replace",
49,
[
"REAL, INTENT(IN) :: input1(xlen, ylen), & !First input array",
" input2(XLEN, ylen), !Second input array",
],
],
# [],
],
0,
{},
),
(
[
["delete", 48, None],
[
"replace",
49,
[
"REAL, INTENT(IN) :: input1(xlen, ylen), & !First input array",
" INPUT2(XLEN, ylen), !Second input array",
],
],
# [],
],
1,
{"Found UPPERCASE variable name in declaration at line 48: \"INPUT2\"" : [48]},
),
([], 0, {}), # No changes, expect no errors
],
ids=[
"2 UpperCase Var Errors",
"False FALSE error",
"5 UpperCase Var Errors on extended lines",
"Array dimnensions, twice on an extended line, but no failures",
"Array dimnensions, twice on an extended line, with one failure",
"No UpperCase Var Errors",
],
)
Expand All @@ -426,11 +477,16 @@ def test_r3_4_2_no_full_uppercase_variable_names(
modified_fortran_lines = modify_fortran_lines(example_fortran_lines, changes_list)
result = r3_4_2_no_full_uppercase_variable_names(modified_fortran_lines)
failure_count = result.failure_count
assert failure_count == expected_result
errors = result.errors
assert len(errors) == len(expected_errors)
for error, lines_list in errors.items():
assert error in expected_errors
assert len(lines_list) == len(expected_errors[error])
for line_no in lines_list:
assert line_no in expected_errors[error]
assert len(lines_list) == len(expected_errors[error])
assert len(errors) == len(expected_errors)
assert failure_count == expected_result


"""TODO: When testing for 'unseparated keywords' remember to test/discard false +ves on
"#endif" or similar. Current (old) test flags these in the wild, but the example file
has no cpp directives..."""
6 changes: 4 additions & 2 deletions script_umdp3_checker/umdp3_checker_rules.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,9 @@ def unseparated_keywords(self, lines: List[str]) -> TestResult:
if line.lstrip(" ").startswith("!"):
continue
clean_line = self.remove_quoted(line)
for pattern in [f"\\b{kw}\\b" for kw in unseparated_keywords_list]:
clean_line = self.comment_line.sub("", clean_line).strip()
# The [^#] is to avoid false positives on #endif and similar.
for pattern in [f"([^#]|^)\\b{kw}\\b" for kw in unseparated_keywords_list]:
if re.search(pattern, clean_line, re.IGNORECASE):
failures += 1
error_log = self.add_error_log(
Expand All @@ -158,7 +160,7 @@ def go_to_other_than_9999(self, lines: List[str]) -> TestResult:
count = -1
for count, line in enumerate(lines):
clean_line = self.remove_quoted(line)
clean_line = re.sub(r"!.*$", "", clean_line)
clean_line = self.comment_line.sub("", clean_line).strip()

if match := re.search(r"\bGO\s*TO\s+(\d+)", clean_line, re.IGNORECASE):
label = match.group(1)
Expand Down
Loading
Loading