Automated gtest formatting checks #3370
Conversation
reidkwja
requested review from
BrianHarrisonAMD,
junliume and
bpepers-me
as code owners
|
|
Will be adding some additional logic to the bash script to ensure better error messages |
| 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")$" | ||
| ) |
There was a problem hiding this comment.
Doesn't it duplicate #3218 ?
There was a problem hiding this comment.
This one extends on it + CI integration that runs upon adding new gtest
There was a problem hiding this comment.
Do we need separate python scripts?
Seems like we might be able to unify these.
Jenkinsfile
Outdated
| when { | ||
| changeset "**/test/utils/**" | ||
| } |
There was a problem hiding this comment.
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?
test/utils/gtest_formating_checks.py
Outdated
| if not TESTSUITE_REGEX.match(suite): | ||
| errors.append(f"{file_path}: Invalid TESTSUITE_NAME '{suite}' in TEST_P.") | ||
|
|
||
| if suite not in instantiated_suites: |
There was a problem hiding this comment.
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.
reidkwja
requested review from
BrianHarrisonAMD,
randyspauldingamd and
CAHEK7
| "../../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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Yes, that a good point... I will add these features here |
Checks for formatting rules in link upon push or changes in gtest directory.