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 TEST_PROPERTIES keyword in ecbuild_add_test #76

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

Conversation

tweska
Copy link

@tweska tweska commented Jan 8, 2025

While updating tests in MultIO, I found that the PROPERTIES keyword could not be used in ecbuild_add_test.

@FussyDuck
Copy link

FussyDuck commented Jan 8, 2025

CLA assistant check
All committers have signed the CLA.

@wdeconinck
Copy link
Member

Hi @tweska welcome to the ecbuild developers team! Great to see contributions.
In short, I have a suggestion to add a new keyword TEST_PROPERTIES instead for this.

The reasoning:

ecbuild_add_test first makes an executable TARGET and then a test invoking that TARGET.
the PROPERTIES to be added to the TARGET is therefore also an acceptable code path, and is not a bug to be fixed.

To add test properties I always did an a posteriori command myself to e.g. add LABELS:

ecbuild_add_test(TARGET <test>)
if (TEST <test>)
  set_tests_properties(<test> PROPERTIES LABELS <labels>)
endif()

I feel that for the very advanced cases that are used in multio to add fixtures etc. it would be OK to use the approach of using set_tests_properties afterwards as shown above?

Note further that there are two other keywords: DEPENDS and TEST_DEPENDS.
DEPENDS is for dependencies between targets for compilation, and TEST_DEPENDS is for dependencies between tests, setting the order of test execution.
We could add the same approach with a new keyword TEST_PROPERTIES for the TEST properties and PROPERTIES could remain for the TARGET. That should then not be a breaking change.

As a side note:
For the more common LABELS case which I am often using I was considering just adding the TEST_LABELS (or just LABELS) keyword to ecbuild_add_test instead, rather than start using "TEST_PROPERTIES LABELS " if it becomes available.
Same way you can set the target's LINKER_LANGUAGE directly without using "PROPERTIES" on the target. We cannot aim to do this for all possible properties of course.

@tweska tweska force-pushed the fix/properties-in-test branch from a37a37e to 1d65e21 Compare January 9, 2025 07:57
@tweska
Copy link
Author

tweska commented Jan 9, 2025

Hi @wdeconinck, I added the TEST_PROPERTIES keyword, as you suggested.

@wdeconinck
Copy link
Member

I noticed the LABELS keyword is already there. I should start using it and rtfm :)

@wdeconinck
Copy link
Member

Nice, could you add a test for this property as well?

@tweska tweska changed the title Fix PROPERTIES in ecbuild_add_test Add TEST_PROPERTIES keyword in ecbuild_add_test Jan 9, 2025
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.

3 participants