-
Notifications
You must be signed in to change notification settings - Fork 283
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
allow use of test_cmd
without runtest
for ConfigureMake
#2837
allow use of test_cmd
without runtest
for ConfigureMake
#2837
Conversation
test_cmd
without runtest
for ConfigureMaketest_cmd
without runtest
for ConfigureMake
Anything missing here? |
@Flamefire Similar to #2838: a battery of tests with existing easyconfigs that either use |
I started test runs for both with some random ECs using ConfigureMake/CMakeMake respectively, hope it doesn't fail for unrelated reasons as I don't expect any issues by these PRs as explained in #2838 (comment) |
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 16 out of 16 (14 easyconfigs in total) |
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
Test report by @akesandgren Overview of tested easyconfigs (in order)
Build succeeded for 1 out of 1 (1 easyconfigs in total) This was with both 2837 and 2838 |
Going in, thanks @Flamefire! |
(created using
eb --new-pr
)When using
CMakeNinja
and running tests one needstest_cmd = 'ctest'
andruntest = ''
which is akward but required because it simply concatsruntest
even when it is not set (None
)Also handle the default case for
test_cmd
in the test function so derived classes can detect if the command was set by the user or not. Required for #2838And finally clean up the executed command by omitting empty values (e.g. pre/posttestopts not set which resulted in
" ctest "
)