Automated gtest formatting checks #3370

reidkwja · 2024-11-05T16:39:15Z

Checks for formatting rules in link upon push or changes in gtest directory.

CAHEK7

I'm just double checking - does the script detects missed INSTANTIATE_TEST_SUITE_P for TEST_P?

junliume · 2024-11-12T19:18:22Z

/home/jenkins/workspace/MLLIBS_MIOpen_rk_gtestchecker@tmp/durable-b196ff28/script.sh: 16: ./test/utils/check_gtests.sh: not found

script returned exit code 127

reidkwja · 2024-11-14T00:15:28Z

Will be adding some additional logic to the bash script to ensure better error messages

CAHEK7 · 2024-11-17T22:33:23Z

test/utils/gtest_formating_checks.py

+VALID_HW_TYPES = {"CPU", "GPU"}
+VALID_DATATYPES = {"FP8", "FP16", "FP32", "FP64", "BFP16", "BFP8", "I64", "I32", "I16", "I8", "NONE"}
+TESTSUITE_REGEX = re.compile(
+    r"^(CPU|GPU)_[A-Za-z0-9]+(?:_[A-Za-z0-9]+)*_(" + "|".join(VALID_DATATYPES) + r")$"
+)


Doesn't it duplicate #3218 ?

This one extends on it + CI integration that runs upon adding new gtest

Do we need separate python scripts?
Seems like we might be able to unify these.

BrianHarrisonAMD · 2024-12-12T23:13:14Z

Jenkinsfile

+                when {
+                    changeset "**/test/utils/**"
+                }


Will this only run if changes happened inside test/utils?

I think we would want this to run if changes happened in changeset "**/test/gtest/**"

Also, did we run it generally on the latest changes to ensure everything is now passing?

randyspauldingamd · 2024-12-13T04:06:17Z

test/utils/gtest_formating_checks.py

+                    if not TESTSUITE_REGEX.match(suite):
+                        errors.append(f"{file_path}: Invalid TESTSUITE_NAME '{suite}' in TEST_P.")
+
+                    if suite not in instantiated_suites:


It's nice to catch these before runtime! It does need to be aware of GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST though, and it also gives false failures when macros are used, e.g. layout_transpose.cpp. I don't think we should discard that flexibility.

randyspauldingamd

Implement GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST, and look into running files through the preprocessor prior to running this script.

CAHEK7 · 2024-12-24T04:44:00Z

test/utils/gtest_formating_checks.py

+    "../../test/gtest/binary_tensor_ops.cpp",
+    "../../test/gtest/layout_transpose.cpp",
+    "../../test/gtest/graphapi_conv_bias_res_add_activ_fwd.cpp",
+    "../../test/gtest/unary_tensor_ops.cpp",


Why is it excluded? It's not an UNINSTANTIATED_PARAMETERIZED_TEST - it is perfectly instantiated.
I guess the problem is in the parsing, since you have to run preprocessor prior to get proper code, but preproceesor will eliminate all INSTANTIATE_TEST_SUITE_P macros.

I would suggest keeping "hard to parse" cases separated from "UNINSTANTIATED_PARAMETERIZED_TEST" cases.

These tests have formatting names that aren't to standard for gtest folder - some are in the macro section, there are regexes like ##SRC_TYPE## in the tests name:

We can either accommodate them or modify them with the standard in different PR:

ERROR: The following issues were found: ERROR: ../../test/gtest/graphapi_operation_rng.cpp: INSTANTIATE_TEST_SUITE_P references non-existent TESTSUITE_NAME 'GraphApiOperationRng'. ERROR: ../../test/gtest/graphapi_operation_rng.cpp: Invalid TEST_TYPE 'InvalidAtLeastSeeds' in INSTANTIATE_TEST_SUITE_P. ERROR: ../../test/gtest/graphapi_conv_bias_res_add_activ_fwd.cpp: Invalid TESTSUITE_NAME 'GPU_ConvBiasResAddActivation_##dir##_##type' in TEST_P. ERROR: ../../test/gtest/graphapi_conv_bias_res_add_activ_fwd.cpp: Test 'GPU_ConvBiasResAddActivation_##dir##_##type.Test' does not have a matching INSTANTIATE_TEST_SUITE_P or GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST. ERROR: ../../test/gtest/binary_tensor_ops.cpp: Invalid TESTSUITE_NAME 'GPU_binaryTensorOps_cast_##SRC_TYPE##_##TEST_TYPE' in TEST_P. ERROR: ../../test/gtest/binary_tensor_ops.cpp: Test 'GPU_binaryTensorOps_cast_##SRC_TYPE##_##TEST_TYPE.\ X_CONCAT_FIRST_SECOND_(__VA_ARGS__, TestTensorCast' does not have a matching INSTANTIATE_TEST_SUITE_P or GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST. ERROR: ../../test/gtest/binary_tensor_ops.cpp: Invalid TESTSUITE_NAME 'GPU_binaryTensorOps_copy_##TEST_TYPE' in TEST_P. ERROR: ../../test/gtest/binary_tensor_ops.cpp: Test 'GPU_binaryTensorOps_copy_##TEST_TYPE.TestTensorCopy' does not have a matching INSTANTIATE_TEST_SUITE_P or GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST. ERROR: ../../test/gtest/unary_tensor_ops.cpp: Invalid TESTSUITE_NAME 'GPU_unaryTensorOps_##TEST_TYPE' in TEST_P. ERROR: ../../test/gtest/unary_tensor_ops.cpp: Test 'GPU_unaryTensorOps_##TEST_TYPE.TestTensorSet' does not have a matching INSTANTIATE_TEST_SUITE_P or GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST. ERROR: ../../test/gtest/layout_transpose.cpp: Invalid TESTSUITE_NAME 'GPU_LayoutTransposeTest_2D_##sol##_##naming_type' in TEST_P. ERROR: ../../test/gtest/layout_transpose.cpp: Test 'GPU_LayoutTransposeTest_2D_##sol##_##naming_type.\ LayoutTransposeTest_2D_##sol##_##type##_P' does not have a matching INSTANTIATE_TEST_SUITE_P or GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST. ERROR: ../../test/gtest/layout_transpose.cpp: Invalid TESTSUITE_NAME 'GPU_LayoutTransposeTest_3D_##sol##_##naming_type' in TEST_P. ERROR: ../../test/gtest/layout_transpose.cpp: Test 'GPU_LayoutTransposeTest_3D_##sol##_##naming_type.\ LayoutTransposeTest_3D_##sol##_##type##_P' does not have a matching INSTANTIATE_TEST_SUITE_P or GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST.

randyspauldingamd

I had some spare cycles so I addressed most of the tests that were being skipped: #3452

I thought I could bypass automatic CI by making a new branch and merging into yours. It didn't work--the CI launched anyway. Well, maybe it'll be useful. Feel free to merge my PR, or merge the branch directly and close it.

After having used the script though, I really wish it emitted line numbers. It would also be nice if it detected and ignored commented code. Is it reasonable to add those features into this PR? I'd also be satisfied with this, and adding those into the next version.

reidkwja · 2025-01-03T16:57:00Z

After having used the script though, I really wish it emitted line numbers. It would also be nice if it detected and ignored commented code. Is it reasonable to add those features into this PR? I'd also be satisfied with this, and adding those into the next version.

Yes, that a good point... I will add these features here

randyspauldingamd

word boundary checks

randyspauldingamd · 2025-01-22T18:25:07Z

test/utils/gtest_formating_checks.py

+TESTSUITE_REGEX = re.compile(
+    r"^(CPU|GPU)_[A-Za-z0-9]+(?:_[A-Za-z0-9]+)*_(" + "|".join(VALID_DATATYPES) + r")$"
+)
+TEST_P_REGEX = re.compile(r"TEST_P\(([^,]+),\s*([^)]+)\)")


Please add the word boundary check to these regexes--it should only be checking valid macro invocations.

TEST_P_REGEX = re.compile(r"\bTEST_P\(([^,]+),\s*([^)]+)\)") INSTANTIATE_TEST_REGEX = re.compile(r"\bINSTANTIATE_TEST_SUITE_P\(\s*([^\n,]+),\s*([^\n,]+),") ALLOW_UNINSTANTIATED_REGEX = re.compile(r"\bGTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST\(\s*([^\)]+)\)")

… rk_gtestchecker

BrianHarrisonAMD

LGTM

randyspauldingamd

line numbers are off
duplicate TEST_P issue
missing comma issue

randyspauldingamd · 2025-01-27T17:56:13Z

test/utils/gtest_formating_checks.py

+                instantiate_matches = [(m.start(), m.groups()) for m in INSTANTIATE_TEST_REGEX.finditer(content)]
+                allow_uninstantiated_matches = [(m.start(), m.group(1)) for m in ALLOW_UNINSTANTIATED_REGEX.finditer(content)]
+
+                # Map line numbers to errors


Line numbers, nice! They aren't quite right though. In all the instances I've seen, they are too small by 1. I also injected an error on line 1 and it printed as line 0, which suggests a counting or indexing issue.

The image also shows another problem I found--if a line is an exact copy of a previous line, one of them is ignored. In this example, line 86 was copied and pasted at line 91 and line 86 was missed. I've also seen cases when 91 was the missing one. You'll probably need to do something along the lines of generating a hash and use that for the key for each line instead of the text itself.

If a comma is missed, the regex consumes everything until the next comma is hit--including CR's. See the error prints for line 88 (printed as line 87). This needs to be fixed in this PR.

This also demonstrates other checks we should add in the future:

TEST: matches our name formatting rules only

TEST_F: matches name format and has a matching TestFixtureName

When a duplicate TEST_P occurs, it should be detected and an error thrown.

Multiple INSTANTIATE_TEST_SUITE_P are allowed for a given TestFixtureName. The INSTANTIATION_NAME (first macro argument) must be unique. Check for this.

Multiple TEST_F are allowed per TestFixtureName. The TestName must be unique. Check for this.

gtest formatting check

625127e

reidkwja marked this pull request as ready for review November 7, 2024 20:00

reidkwja requested review from BrianHarrisonAMD, junliume and bpepers-me as code owners November 7, 2024 20:00

Merge branch 'develop' into rk_gtestchecker

f1303f6

CAHEK7 approved these changes Nov 8, 2024

View reviewed changes

reidkwja added 4 commits November 8, 2024 20:44

clang-fix

34b5365

cmd dir

f6aea6d

scm step

68bdf71

labelsnode

c5e7bc5

reidkwja added 3 commits November 13, 2024 23:01

directory-jenkins

792a89f

debug pwd

a72f2db

fixing pwd in jenkins

7c8309c

reidkwja requested a review from BradPepersAMD as a code owner November 14, 2024 00:11

reidkwja added 6 commits November 15, 2024 00:43

python-gtest-checker

6d2862a

regex+jenkins

55b691d

regex for Test_Type

857353b

jenkins-step-run-test

8639afe

error messaging format

a0ca1a6

testing ci run

6370f44

CAHEK7 suggested changes Nov 17, 2024

View reviewed changes

reidkwja added 2 commits November 18, 2024 22:00

cleanup

28eaf21

Merge branch 'develop' into rk_gtestchecker

5c730bb

reidkwja requested a review from a team as a code owner November 18, 2024 22:01

Merge branch 'develop' into rk_gtestchecker

5bac5d3

BrianHarrisonAMD reviewed Dec 12, 2024

View reviewed changes

randyspauldingamd reviewed Dec 13, 2024

View reviewed changes

randyspauldingamd requested changes Dec 13, 2024

View reviewed changes

Reid Kawaja and others added 5 commits December 13, 2024 16:49

dir fix

7f75e55

allowed uninstantiated suites

63265c4

clang

e60c943

finnalizing + ci run through

afe09d5

Merge branch 'develop' into rk_gtestchecker

a9dd0a3

reidkwja requested review from BrianHarrisonAMD, randyspauldingamd and CAHEK7 December 23, 2024 20:31

CAHEK7 reviewed Dec 24, 2024

View reviewed changes

randyspauldingamd mentioned this pull request Jan 2, 2025

rjs/rk gtc #3452

Open

randyspauldingamd requested changes Jan 2, 2025

View reviewed changes

reidkwja requested review from randyspauldingamd and CAHEK7 January 22, 2025 16:37

Merge branch 'develop' into rk_gtestchecker

331ffbc

randyspauldingamd requested changes Jan 22, 2025

View reviewed changes

reidkwja added 3 commits January 24, 2025 18:47

addressing reviews

36f73c8

Merge branch 'rk_gtestchecker' of https://github.com/ROCm/MIOpen into…

35bccf2

… rk_gtestchecker

Merge branch 'develop' into rk_gtestchecker

0e5f825

reidkwja requested a review from randyspauldingamd January 24, 2025 21:14

BrianHarrisonAMD approved these changes Jan 24, 2025

View reviewed changes

randyspauldingamd requested changes Jan 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automated gtest formatting checks #3370

Automated gtest formatting checks #3370

reidkwja commented Nov 5, 2024

CAHEK7 left a comment

junliume commented Nov 12, 2024

reidkwja commented Nov 14, 2024

CAHEK7 Nov 17, 2024

reidkwja Nov 18, 2024

BrianHarrisonAMD Dec 12, 2024

BrianHarrisonAMD Dec 12, 2024 •

edited

Loading

randyspauldingamd Dec 13, 2024

randyspauldingamd left a comment

CAHEK7 Dec 24, 2024

reidkwja Dec 26, 2024

randyspauldingamd left a comment

reidkwja commented Jan 3, 2025

randyspauldingamd left a comment

randyspauldingamd Jan 22, 2025

BrianHarrisonAMD left a comment

randyspauldingamd left a comment

randyspauldingamd Jan 27, 2025

Automated gtest formatting checks #3370

Are you sure you want to change the base?

Automated gtest formatting checks #3370

Conversation

reidkwja commented Nov 5, 2024

CAHEK7 left a comment

Choose a reason for hiding this comment

junliume commented Nov 12, 2024

reidkwja commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrianHarrisonAMD Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randyspauldingamd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randyspauldingamd left a comment

Choose a reason for hiding this comment

reidkwja commented Jan 3, 2025

randyspauldingamd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrianHarrisonAMD left a comment

Choose a reason for hiding this comment

randyspauldingamd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrianHarrisonAMD Dec 12, 2024 •

edited

Loading