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

[1.0.3] fix eosvmoc_limits_tests on non-x86 and non-Linux (where OC is not supported) #904

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

spoonincode
Copy link
Member

This is a replacement for AntelopeIO/leap#2392 which was not accepted prior to that release going in to another support phase.

Recall that the way ctest tests are populated for unit_test are by finding all the .cpp files in the unittests directory,

file(GLOB UNIT_TESTS "*.cpp") # find all unit test suites

sending those through a grep to find the TEST_SUITE() for each,
execute_process(COMMAND sh -c "grep -E 'BOOST_AUTO_TEST_SUITE\\s*[(]' '${TEST_SUITE}' | grep -vE '//.*BOOST_AUTO_TEST_SUITE\\s*[(]' | cut -d ')' -f 1 | cut -d '(' -f 2" OUTPUT_VARIABLE SUITE_NAMES OUTPUT_STRIP_TRAILING_WHITESPACE) # get the test suite name from the *.cpp file

and then adding a test on what it find, for each WASM runtime enabled,
foreach(RUNTIME ${EOSIO_WASM_RUNTIMES})
add_test(NAME ${TRIMMED_SUITE_NAME}_unit_test_${RUNTIME} COMMAND unit_test --run_test=${SUITE_NAME} --report_level=detailed --color_output -- --${RUNTIME})

Notice how previously the entire eosvmoc_limits_tests.cpp is #ifdefed away when OC is not enabled. The glob+grep above has no knowledge of this, so when OC is not enabled the cmake file generates a ctest entry that runs unit_test --run_test=eosvmoc_limits_tests but that will always fail because there is no test suite named eosvmoc_limits_tests.

Adjust the eosvmoc_limits test suite and tests to be defined no matter OC enabled or not, and just #ifdef out the small OC-specific part.

This change still looks strange: why are we running these OC limits tests at all with OC is not enabled for the build? Well, that's actually what is occurring even when OC is enabled normally on our x86 Linux builds. For example this previous run you can see that we run the eosvmoc_limits test even for non-OC runtimes:
https://github.com/AntelopeIO/spring/actions/runs/11222323498/job/31195622799#step:5:227
It's just that when OC isn't enabled limit_violated_test() always passes. So arguably this change is more consistent with what already occurs when OC builds are enabled vs, say, stubbing out a no-op test suite and test when OC build is not enabled. Any more complex changes to how tests are populated are probably not reasonably in scope for a release branch.

@greg7mdp
Copy link
Contributor

greg7mdp commented Oct 8, 2024

I think we should run the grep om the preprocessed unit test file.

@spoonincode
Copy link
Member Author

Instead of doubling down on greping source files -- preprocessed or not -- I've wondered about making our own dispatcher instead. ctest would run our dispatcher, which would run unit_test --list-content to figure out what tests there are, and then run those in multiple processes; maybe with some grouping policy.

There are some benefits with doing this, such as not needing the part1, part2, part3 stuff, as well as likely achieving better parallelism which will make the tests complete faster.

My main concern is goofing up the dispatcher and accidentally not running some tests which goes unnoticed.

@ericpassmore ericpassmore added the test-bug A test is not working as was intended. label Oct 8, 2024
@ericpassmore
Copy link
Contributor

Note:start
category: Tests
component: Internal
summary: Fix a bug where eos vm tests where not activated when OC was not supported or not activated.
Note:end

@spoonincode spoonincode merged commit 97dc35e into release/1.0 Oct 9, 2024
36 checks passed
@spoonincode spoonincode deleted the eosvmoc_arm_etc_limit_tests_10 branch October 9, 2024 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test-bug A test is not working as was intended.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants