Skip to content

[GeoMechanicsApplication] Improved handling of unit tests that need to run conditionally#13677

Merged
avdg81 merged 2 commits intomasterfrom
geo/skip-unit-tests-if-needed
Jul 30, 2025
Merged

[GeoMechanicsApplication] Improved handling of unit tests that need to run conditionally#13677
avdg81 merged 2 commits intomasterfrom
geo/skip-unit-tests-if-needed

Conversation

@avdg81
Copy link
Contributor

@avdg81 avdg81 commented Jul 28, 2025

📝 Description

Use GTEST_SKIP rather than not compiling unit tests that need to be run conditionally. So far, several unit tests are skipped when non-debug builds are used.

With these changes, the total number of unit tests that is run is identical for all build types. However, the number of skipped tests may differ.

Use `GTEST_SKIP` rather than not compiling unit tests that need to be run conditionally. So far, several unit tests are skipped when non-debug builds are used.

With these changes, the total number of unit tests that is run is identical for all build types. However, the number of skipped tests may differ.
@avdg81 avdg81 requested review from markelov208 and rfaasse July 28, 2025 07:25
@avdg81 avdg81 self-assigned this Jul 28, 2025
@avdg81 avdg81 requested a review from a team as a code owner July 28, 2025 07:25
@avdg81 avdg81 added the GeoMechanics Issues related to the GeoMechanicsApplication label Jul 28, 2025
Copy link
Contributor

@rfaasse rfaasse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up. In principle I agree with the changes, but I'm not too sure if it makes it clear e.g. in the pipelines why they are skipped:

image image

Also locally, you'll always get these skipped test cases at the end (like in the first picture) without a reason being printed. Maybe we should add clearly to the names of the test that they should do something only in debug?

The new names give a hint about the reason why the tests were skipped.
@avdg81
Copy link
Contributor Author

avdg81 commented Jul 29, 2025

Thanks for picking this up. In principle I agree with the changes, but I'm not too sure if it makes it clear e.g. in the pipelines why they are skipped:
image image

Also locally, you'll always get these skipped test cases at the end (like in the first picture) without a reason being printed. Maybe we should add clearly to the names of the test that they should do something only in debug?

Thanks a lot for your suggestion. I have renamed the test cases that are potentially skipped, to give a hint about the reason why they were skipped. Does this help in your opinion?

@avdg81 avdg81 requested a review from rfaasse July 29, 2025 15:29
Copy link
Contributor

@markelov208 markelov208 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Anne, thank you for the PR. GTEST_SKIP is better and clear approach. Do we a have a style to name unit tests?

@github-project-automation github-project-automation bot moved this from In progress to Reviewer approved in GeoMechanicsApplication (Deltares) Jul 30, 2025
Copy link
Contributor

@rfaasse rfaasse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now indeed it's clear why these tests would've been skipped from the logging, thanks for this improvement!

@avdg81
Copy link
Contributor Author

avdg81 commented Jul 30, 2025

Thanks for picking this up. In principle I agree with the changes, but I'm not too sure if it makes it clear e.g. in the pipelines why they are skipped:
image image

Also locally, you'll always get these skipped test cases at the end (like in the first picture) without a reason being printed. Maybe we should add clearly to the names of the test that they should do something only in debug?

Thanks a lot for your suggestion. I have renamed the test cases that are potentially skipped, to give a hint about the reason why they were skipped. Does this help in your opinion?

Hi Anne, thank you for the PR. GTEST_SKIP is better and clear approach. Do we a have a style to name unit tests?

Good question. I'm not aware of any naming style for the unit tests, but perhaps we should think of one. What do you think?

@avdg81 avdg81 merged commit 464819e into master Jul 30, 2025
10 checks passed
@avdg81 avdg81 deleted the geo/skip-unit-tests-if-needed branch July 30, 2025 07:18
@github-project-automation github-project-automation bot moved this from Reviewer approved to Done in GeoMechanicsApplication (Deltares) Jul 30, 2025
@markelov208
Copy link
Contributor

Hi Anne, thank you for the PR. GTEST_SKIP is better and clear approach. Do we a have a style to name unit tests?

Good question. I'm not aware of any naming style for the unit tests, but perhaps we should think of one. What do you think?

Sure, it will be nice to have a certain style. In other places a class name is used and underscore to improve readability. It can be more things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

GeoMechanics Issues related to the GeoMechanicsApplication

Development

Successfully merging this pull request may close these issues.

[GeoMechanicsApplication] Skip rather than not compile unit tests that are intended to be run using debug builds only

3 participants