Added basic architecture for common tests #357

ahcorde · 2022-06-15T14:18:11Z

Signed-off-by: ahcorde [email protected]

🎉 New feature

Summary

This PR is a simplifycation of this other PR #354

This draft PR try to set up a common arquitecture to test all physics engine under the same tests avoiding duplication and improving coverage.

Test it

colcon test --merge-install --event-handlers console_direct+ --packages-select ignition-physics6 --ctest-args -R COMMON

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)
Updated migration guide (as needed)
Consider updating Python bindings (if the library has them)
codecheck passed (See contributing)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: ahcorde <[email protected]>

mjcarroll · 2022-06-15T14:23:48Z

I like this, it's very much in line with what we discussed.

@scpeters what do you think?

Signed-off-by: ahcorde <[email protected]>

codecov · 2022-06-15T15:01:24Z

Codecov Report

Merging #357 (fd9ddc2) into main (e51974d) will decrease coverage by 0.91%.
The diff coverage is 3.94%.

❗ Current head fd9ddc2 differs from pull request most recent head 8510048. Consider uploading reports for the commit 8510048 to get more accurate results

@@            Coverage Diff             @@
##             main     #357      +/-   ##
==========================================
- Coverage   75.46%   74.55%   -0.92%     
==========================================
  Files         128      128              
  Lines        5552     5628      +76     
==========================================
+ Hits         4190     4196       +6     
- Misses       1362     1432      +70

Impacted Files	Coverage Δ
bullet/src/EntityManagementFeatures.cc	`9.09% <3.94%> (-5.84%)`	⬇️
dartsim/src/EntityManagementFeatures.cc	`79.47% <0.00%> (+0.99%)`	⬆️

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 e51974d...8510048. Read the comment docs.

scpeters

This approach reminds me of the approach in gazebo11, which has a hard-coded list of physics engines to use. I've been envisioning an executable like the hello_world_loader that takes the path to a physics plugin as an input and uses FindFeatures::From to identify the plugin names within that file that implement the specified FeatureList. The main difference from hello_world_loader is that it would be a gtest executable (if possible) and run test expectations against the APIs implemented by the plugin.

Eventually, I had imagined that this test executable could be installed somewhere not on the PATH (like libexec) and exposed as part of an ign physics test command for use by 3rd-party physics plugin developers, but that we would also have cmake logic to add tests that run this test executable directly with paths to the dartsim, bullet, and tpe-plugin binaries in the build folder.

scpeters · 2022-06-16T10:31:49Z

test/test_common_config.h.in

+    gz::physics::TestValues())
+
+static const std::vector<const char *> kPhysicsEngineTestValues
+  {"bullet", "dartsim", "tpe"};


this reminds me of gazebo11's approach

scpeters · 2022-06-16T10:32:02Z

test/common_test/basic_test.cc

+}
+
+INSTANTIATE_TEST_CASE_P(EntityManagementFeatures, EntityManagementFeaturesTest,
+    PHYSICS_ENGINE_VALUES,


this reminds me of gazebo11's approach

Signed-off-by: ahcorde <[email protected]>

…om/ignitionrobotics/ign-physics into ahcorde/common_test_initial_commit

ahcorde · 2022-06-17T07:44:33Z

@scpeters what do you think about this logic ?

Signed-off-by: ahcorde <[email protected]>

scpeters

environment variables are a little easier to set from cmake, but if possible I think it would be nice to use CLI arguments to specify the library name and optional plugin name. We can save that for a later refactoring though

scpeters · 2022-06-17T12:55:24Z

test/common_test/basic_test.cc

+  {
+    gz::common::Console::SetVerbosity(4);
+
+    if (!gz::common::env("PHYSICS_ENGINE_NAME", physicsEngineName))


I think we shouldn't need to specify PHYSICS_ENGINE_NAME. If it's not specified, we should use FindFeatures3d to identify the plugin names contained within the LIB_TO_TEST and then test each of those plugins. To support this, we should get the list of pluginNames in SetUp and move the gz::plugin::Loader instantiation and LoadLib call to a loop in the test case

gz-physics/examples/hello_world_loader/hello_world_loader.cc

Lines 51 to 63 in e91c58a

// Look for 3d plugins

auto pluginNames = ignition::physics::FindFeatures3d<Features>::From(pl);

if (pluginNames.empty())

{

std::cerr << "No plugins with required features found in "

<< pluginPath

<< std::endl;

}

for (const std::string &name : pluginNames)

{

std::cout << "Testing plugin: " << name << std::endl;

ignition::plugin::PluginPtr plugin = pl.Instantiate(name);

Signed-off-by: ahcorde <[email protected]>

ahcorde · 2022-06-17T14:53:42Z

@scpeters I'm not really sure about adding command line arguments to gtest. I will take a look

ahcorde · 2022-06-17T15:35:47Z

@scpeters I added this draft PR #359 with the changes required to use cmd line arguments

scpeters

this is great, and I also like the changes in #357

scpeters · 2022-06-17T23:20:32Z

test/common_test/basic_test.cc

+
+class EntityManagementFeaturesTest:
+  public testing::Test,
+  public testing::WithParamInterface<const char *>


nit: we aren't currently using parameterized tests (TEST_P) so this WithParamInterface line can be removed

scpeters · 2022-06-17T23:20:49Z

test/common_test/basic_test.cc

+#include <gtest/gtest.h>
+
+#include <gz/common/Filesystem.hh>
+#include <gz/common/Console.hh>


nit: alphabetize

scpeters · 2022-06-17T23:24:39Z

test/common_test/basic_test.cc

+
+    auto plugins = loader.LoadLib(libToTest);
+
+    pluginNames = gz::physics::FindFeatures3d<Features>::From(loader);


future todo: we should also run the 3f, 2d, and 2f variants of FindFeatures

Signed-off-by: ahcorde <[email protected]>

ahcorde · 2022-06-20T16:52:06Z

@scpeters Do you mind to review this PR #359 which built on top of this one?

scpeters · 2022-06-20T17:57:39Z

@scpeters Do you mind to review this PR #359 which built on top of this one?

I just approved it 😄

…nts (#359) * Added basic architecture for common tests with cmd arguments Signed-off-by: ahcorde <[email protected]>

Added basic architecture for common tests

30f463e

Signed-off-by: ahcorde <[email protected]>

ahcorde added DART DART engine TPE Trivial Physics Engine Bullet Bullet engine labels Jun 15, 2022

ahcorde self-assigned this Jun 15, 2022

ahcorde requested a review from mjcarroll June 15, 2022 14:18

github-actions bot added the 🌱 garden Ignition Garden label Jun 15, 2022

make linters happy

dd6f502

Signed-off-by: ahcorde <[email protected]>

Merge branch 'main' into ahcorde/common_test_initial_commit

a9ee0cc

ahcorde marked this pull request as ready for review June 15, 2022 15:36

ahcorde requested review from azeey, mxgrey and scpeters as code owners June 15, 2022 15:36

scpeters reviewed Jun 16, 2022

View reviewed changes

ahcorde added 2 commits June 16, 2022 21:11

improve cmake logic

481e6c9

Signed-off-by: ahcorde <[email protected]>

Merge branch 'ahcorde/common_test_initial_commit' of https://github.c…

1a3cfd4

…om/ignitionrobotics/ign-physics into ahcorde/common_test_initial_commit

ahcorde requested a review from scpeters June 17, 2022 07:44

Fixed CI

4b0005c

Signed-off-by: ahcorde <[email protected]>

scpeters reviewed Jun 17, 2022

View reviewed changes

Added feedback

1f48f3a

Signed-off-by: ahcorde <[email protected]>

scpeters reviewed Jun 17, 2022

View reviewed changes

ahcorde added 2 commits June 20, 2022 15:43

Added feedback

02e62fa

Signed-off-by: ahcorde <[email protected]>

Merge branch 'main' into ahcorde/common_test_initial_commit

e2e4e62

ahcorde requested a review from scpeters June 20, 2022 13:44

scpeters approved these changes Jun 20, 2022

View reviewed changes

Suggestion: Added basic architecture for common tests with cmd argume…

8510048

…nts (#359) * Added basic architecture for common tests with cmd arguments Signed-off-by: ahcorde <[email protected]>

ahcorde merged commit a950315 into main Jun 20, 2022

ahcorde deleted the ahcorde/common_test_initial_commit branch June 20, 2022 19:10

ahcorde mentioned this pull request Jun 29, 2022

[Meta-Ticket] Generalize tests #372

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added basic architecture for common tests #357

Added basic architecture for common tests #357

ahcorde commented Jun 15, 2022

mjcarroll commented Jun 15, 2022

codecov bot commented Jun 15, 2022 •

edited

Loading

scpeters left a comment

scpeters Jun 16, 2022

scpeters Jun 16, 2022

ahcorde commented Jun 17, 2022 •

edited

Loading

scpeters left a comment

scpeters Jun 17, 2022

ahcorde Jun 17, 2022

ahcorde commented Jun 17, 2022

ahcorde commented Jun 17, 2022

scpeters left a comment

scpeters Jun 17, 2022

ahcorde Jun 20, 2022

scpeters Jun 17, 2022

ahcorde Jun 20, 2022

scpeters Jun 17, 2022

ahcorde Jun 20, 2022

ahcorde commented Jun 20, 2022

scpeters commented Jun 20, 2022

	// Look for 3d plugins
	auto pluginNames = ignition::physics::FindFeatures3d<Features>::From(pl);
	if (pluginNames.empty())
	{
	std::cerr << "No plugins with required features found in "
	<< pluginPath
	<< std::endl;
	}

	for (const std::string &name : pluginNames)
	{
	std::cout << "Testing plugin: " << name << std::endl;
	ignition::plugin::PluginPtr plugin = pl.Instantiate(name);


		auto plugins = loader.LoadLib(libToTest);

		pluginNames = gz::physics::FindFeatures3d<Features>::From(loader);

Added basic architecture for common tests #357

Added basic architecture for common tests #357

Conversation

ahcorde commented Jun 15, 2022

🎉 New feature

Summary

Test it

Checklist

mjcarroll commented Jun 15, 2022

codecov bot commented Jun 15, 2022 • edited Loading

Codecov Report

scpeters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahcorde commented Jun 17, 2022 • edited Loading

scpeters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahcorde commented Jun 17, 2022

ahcorde commented Jun 17, 2022

scpeters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahcorde commented Jun 20, 2022

scpeters commented Jun 20, 2022

codecov bot commented Jun 15, 2022 •

edited

Loading

ahcorde commented Jun 17, 2022 •

edited

Loading