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

EkatCreateUnitTest still expects the test sources, even if TEST_EXE is provided #165

Closed
bartgol opened this issue Nov 19, 2021 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@bartgol
Copy link
Contributor

bartgol commented Nov 19, 2021

Describe the bug
The signature of the macro is

function(EkatCreateUnitTest test_name test_srcs)

which means that the test sources are mandatory. When providing a test exec, it makes no sense to force the user to provide sources. Although possible in our current use cases, in a more general context, one might want to create the exec once in a "root" folder, then use it several times in subfolders, to create unit tests with different setup/configs, but same exec.

A possible solution would be to make sources optional, with a keyword, as in EkatCreateUnitTest(my_test SOURCES <src list> ...). This is not backward compatible, though it might be more intuitive? Another solution is to create more macros (that recycle one another):

# create exec
macro (EkatCreateUnitTestExec exec_name test_srcs)
  # create the exec (e.g., take care of catch2 stuff)
endmacro ()

# create tests
macro (EkatCreateUnitTestFromExec test_name test_exec)
  # Given a test exec, create the unit tests combination (e.g., THREADS, RANKS, valg...)
endmacro()

# create exec and tests
macro (EkatCreateUnitTest test_name test_srcs)
   EkatCreateUnitTestExec ($test_name $test_srcs <other optional args, like libs, flags>)
   EkatCreateUnitTestFromExec ($test_name $test_name <other optional args, like RANKS, THREADS, valg>)
endmacro()

This would be backward compatible.

@bartgol bartgol added the bug Something isn't working label Nov 19, 2021
@jgfouca
Copy link
Member

jgfouca commented Nov 19, 2021

@bartgol , I noticed this when I was testing this feature. The user just needs to pass an empty string for those, like I did here:

  CreateUnitTest(resource_spread_rank "" ""
    TEST_EXE resource_spread_thread

Not super elegant, but at least it keeps us from needing multiple macros. One thing we could do is raise an error if the user provides a TEST_EXE but also tries to provide sources or test libs.

We could bite the bullet and go with multiple macros too.

@bartgol
Copy link
Contributor Author

bartgol commented Nov 22, 2021

Imho, passing empty args just to fit into the existing macro is not the right choice. You're hijacking the existing framework to do something different. For a developer that has never seen it, it might be confusing.

I think I will write the 3 macro solution, which is fully bwd compatible, and (imho) more self-explanatory for the new use case. Furthermore, since the last macro (the one that we currently use) would be impl-ed in terms of the previous two, I think the amount of actual code (adding up the macros bodies) would remain unchanged. It would simply be broken into 2 macros.

@jgfouca
Copy link
Member

jgfouca commented Nov 24, 2021

Fixed by #166

@jgfouca jgfouca closed this as completed Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants