-
Notifications
You must be signed in to change notification settings - Fork 10
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
add common method for registering different test types #1168
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like an improvement to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
endfunction() | ||
|
||
function(add_p_test) | ||
cmake_parse_arguments(PARSE_ARGV 0 arg "" "NAME;COST;TIMEOUT" "COMMAND") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this cmake_parse_arguments
duplicate as setup_test_common
calls it too?
add_test(NAME ${TRIMMED_SUITE_NAME}_unit_test_${RUNTIME}-nofsgs COMMAND unit_test --run_test=${SUITE_NAME} --report_level=detailed --color_output -- --${RUNTIME}) | ||
set_tests_properties(${TRIMMED_SUITE_NAME}_unit_test_${RUNTIME}-nofsgs PROPERTIES ENVIRONMENT "SPRING_DISABLE_FSGSBASE=1" COST 5000) | ||
add_p_test(NAME ${TRIMMED_SUITE_NAME}_unit_test_${RUNTIME}-nofsgs COMMAND unit_test --run_test=${SUITE_NAME} --report_level=detailed --color_output -- --${RUNTIME} COST 5000) | ||
set_tests_properties(${TRIMMED_SUITE_NAME}_unit_test_${RUNTIME}-nofsgs PROPERTIES ENVIRONMENT "SPRING_DISABLE_FSGSBASE=1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is COST 5000
moved up?
Note:start |
This is a change I was required to make for #1000, but with that effort being buried I thought I'd still cherry pick this change out for opinions on taking it stand alone.
This removes a lot of boilerplate when declaring tests (especially LR & NP tests) by removing, for example,
and replacing it with
There is
add_p_test()
,add_np_test()
,add_lr_test()
, andadd_wasmspec_test()
that all do what you'd expect. Any place that continues to just useadd_test()
(like in a submodule) will still just keep working as expected.add_p_test()
is probably the most questionable, as it adds little utility over justadd_test()
since the parallelizable tests don't need a label. Though it does remove theWORKING_DIRECTORY ${CMAKE_BINARY_DIR}
boilerplate used in spots.Certainly a big fear with a change like this is inadvertently disabling or altering a test somehow. To ensure no test left behind, one thing we can look at is the number of tests printed in the CI run compared to last run on
main
. This is easiest to compare for parallelizable tests since there is a number printed at the end. Agrep | wc
can be used on the log files of NP & LR tests to ensure the count matches up.Another more through check is to create json output from ctest before and after the change,
Unfortunately there is a bunch of changes in the json due to "backtrace" related stuff (a graph of tests), so filter that out,
Now can do a
diff -u1 before-filtered.json after-filtered.json
. Unfortunately again this still a bit large because of changes such asAt this point I just scrolled through the output and manually verified the only thing in the diff was
WORKING_DIRECTORY
changes (due to the commonWORKING_DIRECTORY "${CMAKE_BINARY_DIR}"
now present that wasn't the case with every test in the past). Clearly the working directory being switch from, for example,/spring/build/unittests/wasm-spec-tests/generated-tests
to/spring/build
hasn't affected the ability of the test to run.One other minor annoyance with this change I noticed is that the indirection removes the▶️ from CLion's editor next to the
add_test()
to easily run the test.