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.

Sign up for GitHub

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

Jump to bottom

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 •
edited
Loading

🎉 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 •
edited
Loading

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.

@mjcarroll
Add explicit root variant of TempDirectory
78e0110 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Add common::testing module
8d01233 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll mjcarroll mentioned this pull request Feb 24, 2022
Merged
7 tasks
@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 mentioned this pull request Mar 3, 2022
Merged
@chapulina chapulina added the tests Broken or missing tests / testing infra label Mar 3, 2022
chapulina
chapulina reviewed Mar 12, 2022
View reviewed changes
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/BazelTestPaths.hh Show resolved Hide resolved
testing/include/ignition/common/testing/CMakeTestPaths.hh Show resolved Hide resolved
testing/include/ignition/common/testing/TestPaths.hh 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
Address reviewer feedback
4d1246c 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Merge branch 'main' into add_common_testing
85654e4
@mjcarroll
Fixup AutoLogFixture and tests that had transitive depends
377e67d 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@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

nkoenig
nkoenig reviewed Apr 5, 2022
View reviewed changes
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/include/ignition/common/testing/detail/AutoLogFixture.hh Show resolved Hide resolved
testing/src/TestPaths.cc Outdated Show resolved Hide resolved
@mjcarroll mjcarroll mentioned this pull request Apr 5, 2022
Merged
@mjcarroll
Copy link
Contributor Author

#337 should fix the Windows CI

@azeey azeey removed their request for review April 5, 2022 16:11
@mjcarroll
Fix visibility for Windows
7825749 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Visibility for unit tests
f094cad 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Fix 'using' visibility
84dbb6c 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Merge branch 'main' into add_common_testing
ff51ca1
@mjcarroll
Add checks and tests for empty _root parameter
b2f7480 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Add documentation to projectSourcePath
bab3599 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Remove getRandomNumber in favor of ign-math impl
44cce83 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Add more ASSERTs to AutoLogFixture
9c3b6dc 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Warn when both bazel and cmake detected
046cf5b 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Lintt
303928d 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@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 chapulina mentioned this pull request Apr 15, 2022
Merged
@chapulina
Copy link
Contributor

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

@nkoenig nkoenig nkoenig approved these changes

@chapulina chapulina Awaiting requested review from chapulina

Assignees

@mjcarroll mjcarroll

Labels
🌱 garden Ignition Garden tests Broken or missing tests / testing infra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mjcarroll @scpeters @chapulina @nkoenig