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 common::testing module #314

Merged
merged 20 commits into from
Apr 11, 2022
Merged

Add common::testing module #314

merged 20 commits into from
Apr 11, 2022

Conversation

mjcarroll
Copy link
Contributor

@mjcarroll mjcarroll commented Feb 23, 2022

🎉 New feature

Summary

This introduces ignition::common::testing, a component to hold all of our testing support functionality.

The main motivation is to do away with test_config.h.in files across our components, and provide a more reliable way of getting source files, test support files, and temporary directories from the file system.

This should end up with any uses of PROJECT_SOURCE_PATH or CMAKE_SOURCE_PATH removed from our codebase in a centralized module.

I also envision this holding other common functionality, such as forking executables as part of a test.

Test it

See #315 for how this is used

Requires gazebosim/gz-cmake/pull/206

@mjcarroll mjcarroll requested a review from azeey February 23, 2022 14:55
@mjcarroll mjcarroll self-assigned this Feb 23, 2022
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Feb 23, 2022
@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #314 (210641e) into main (8761b6f) will decrease coverage by 0.00%.
The diff coverage is 77.50%.

@@            Coverage Diff             @@
##             main     #314      +/-   ##
==========================================
- Coverage   77.14%   77.13%   -0.01%     
==========================================
  Files          76       84       +8     
  Lines       10547    10661     +114     
==========================================
+ Hits         8136     8223      +87     
- Misses       2411     2438      +27     
Impacted Files Coverage Δ
.../include/ignition/common/testing/BazelTestPaths.hh 0.00% <0.00%> (ø)
testing/src/BazelTestPaths.cc 0.00% <0.00%> (ø)
testing/src/Utils.cc 30.00% <30.00%> (ø)
testing/src/TestPaths.cc 76.92% <76.92%> (ø)
av/src/AudioDecoder.cc 83.33% <100.00%> (+0.16%) ⬆️
src/TempDirectory.cc 78.87% <100.00%> (+2.31%) ⬆️
.../include/ignition/common/testing/CMakeTestPaths.hh 100.00% <100.00%> (ø)
...sting/include/ignition/common/testing/TestPaths.hh 100.00% <100.00%> (ø)
...e/ignition/common/testing/detail/AutoLogFixture.hh 100.00% <100.00%> (ø)
testing/src/CMakeTestPaths.cc 100.00% <100.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8761b6f...210641e. Read the comment docs.

@scpeters
Copy link
Member

should this go in ign-utils so it can be used by ign-math and ign-plugin?

@mjcarroll
Copy link
Contributor Author

should this go in ign-utils so it can be used by ign-math and ign-plugin?

I think ign-utils is pretty safe because it doesn't have a lot of need for it.

In theory we could make the dependency from ign-plugin to ign-common purely for the tests, but this really only works if you are building an entire source workspace with colcon or bazel rather than depending on packaged binaries.

@mjcarroll
Copy link
Contributor Author

ign-plugin also circumvents this a bit by directly injecting paths to libraries in the tests via preprocessor definitions. It works fine for a few examples and is probably okay because plugin assumes that most of the path munging will have already been handed by common.

@mjcarroll
Copy link
Contributor Author

@osrf-jenkins retest this please

@chapulina chapulina added the tests Broken or missing tests / testing infra label Mar 3, 2022
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Did a 1st pass

include/ignition/common/TempDirectory.hh Show resolved Hide resolved
testing/include/ignition/common/testing/AutoLogFixture.hh Outdated Show resolved Hide resolved
testing/include/ignition/common/testing/AutoLogFixture.hh Outdated Show resolved Hide resolved
testing/include/ignition/common/testing/Utils.hh Outdated Show resolved Hide resolved
testing/src/BazelTestPaths_TEST.cc Outdated Show resolved Hide resolved
testing/src/CMakeTestPaths.cc Outdated Show resolved Hide resolved
testing/src/CMakeTestPaths.cc Outdated Show resolved Hide resolved
@mjcarroll
Copy link
Contributor Author

@osrf-jenkins retest this please

@chapulina chapulina self-requested a review March 28, 2022 18:49
@chapulina chapulina requested a review from nkoenig April 4, 2022 18:31
@mjcarroll
Copy link
Contributor Author

@osrf-jenkins retest this please

src/TempDirectory.cc Show resolved Hide resolved
testing/include/ignition/common/testing/TestPaths.hh Outdated Show resolved Hide resolved
testing/include/ignition/common/testing/Utils.hh Outdated Show resolved Hide resolved
testing/src/TestPaths.cc Outdated Show resolved Hide resolved
@mjcarroll
Copy link
Contributor Author

#337 should fix the Windows CI

@azeey azeey removed their request for review April 5, 2022 16:11
@mjcarroll mjcarroll requested a review from nkoenig April 5, 2022 20:36
@mjcarroll mjcarroll merged commit 96df813 into main Apr 11, 2022
@mjcarroll mjcarroll deleted the add_common_testing branch April 11, 2022 12:32
@scpeters
Copy link
Member

we need to update the ign-common5-release repo with this new component, similar to gazebo-release/gz-common5-release#4

@mjcarroll
Copy link
Contributor Author

we need to update the ign-common5-release repo with this new component, similar to gazebo-release/gz-common5-release#4

Will do, thanks

@chapulina
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden tests Broken or missing tests / testing infra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants