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

add common method for registering different test types #1168

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

spoonincode
Copy link
Member

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,

add_test(NAME nodeos_sanity_test COMMAND tests/nodeos_run_test.py -v --sanity-test ${UNSHARE} WORKING_DIRECTORY ${CMAKE_BINARY_DIR})
set_property(TEST nodeos_sanity_test PROPERTY LABELS nonparallelizable_tests)

and replacing it with

add_np_test(NAME nodeos_run_test COMMAND tests/nodeos_run_test.py -v ${UNSHARE})

There is add_p_test(), add_np_test(), add_lr_test(), and add_wasmspec_test() that all do what you'd expect. Any place that continues to just use add_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 just add_test() since the parallelizable tests don't need a label. Though it does remove the WORKING_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. A grep | 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,

ctest --show-only=json-v1 > before.json
ctest --show-only=json-v1 > after.json

Unfortunately there is a bunch of changes in the json due to "backtrace" related stuff (a graph of tests), so filter that out,

cat before.json | jq 'del(.version)|del(.backtraceGraph)|del(.tests[].backtrace)' > before-filtered.json
cat after.json | jq 'del(.version)|del(.backtraceGraph)|del(.tests[].backtrace)' > after-filtered.json

Now can do a diff -u1 before-filtered.json after-filtered.json. Unfortunately again this still a bit large because of changes such as

@@ -46872,3 +46872,3 @@
           "name": "WORKING_DIRECTORY",
-          "value": "/spring/build/unittests/wasm-spec-tests/generated-tests"
+          "value": "/spring/build"
         }

At this point I just scrolled through the output and manually verified the only thing in the diff was WORKING_DIRECTORY changes (due to the common WORKING_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.

Copy link
Member

@heifner heifner left a 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.

Copy link
Contributor

@greg7mdp greg7mdp left a 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")
Copy link
Member

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")
Copy link
Member

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?

@ericpassmore
Copy link
Contributor

Note:start
category: Tests
component: Internal
summary: Improve test helpers by adding common method for registering different test types.
Note:end

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.

5 participants