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

Expose a test fixture helper class #926

Merged
merged 2 commits into from
Aug 4, 2021
Merged

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Jul 21, 2021

🎉 New feature

Summary

It's not always straightforward to write automated tests that leverage ign-gazebo from external projects. Internal to Gazebo, we have various helper classes in test/helpers that make it easier to write integration tests. The goal of this PR is to expose a public API that makes it easier to write tests similar to those.

The main class being added in this PR is the TestFixture. See the added example for how to use it in an external project together with GTest. Example usage from the classes's docs:

/// // Load a world with a fixture
/// ignition::gazebo::TestFixture fixture("path_to.sdf");
///
/// // Register callbacks, for example:
/// fixture.OnPostUpdate([&](const gazebo::UpdateInfo &,
///   const gazebo::EntityComponentManager &_ecm)
///   {
///     // Add expectations here
///   }
///
/// // Run the server
/// fixture.Server()->Run(true, 1000, false);

Besides test/helpers, a lot of the inspiration here is coming from @arjo129 's work on a separate project.

Differences from test/helpers

While working on the public API I made some improvements over what we are currently using internally:

  • Added new Server::AddSystem APIs that allow us to load a plugin created on the fly, instead of needing to load a shared library. (i.e. no more need for libMockPlugin.so, see the HelperSystem instead)
  • The TestFixture, unlike test/helpers/Relay, instantiates a server automatically. This saves even more boilerplate in test code.

Test it

Run the new standalone example. I made an effort to use classes like World and Model on the example test in order to make it as approachable as possible. (i.e. no Each calls)

TODO

TODO before merging:

  • Add tests for Server::AddSystem
  • Add tests for TestFixture
  • I'd like to add a 2nd example test that is more realistic, sending commands and reading sensor data
  • General cleanup (docs, style, etc)

TODO in the future:

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

@chapulina chapulina requested a review from arjo129 July 21, 2021 23:07
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Jul 21, 2021
examples/standalone/gtest_setup/gravity.sdf Outdated Show resolved Hide resolved
include/ignition/gazebo/TestFixture.hh Outdated Show resolved Hide resolved
src/TestFixture.cc Outdated Show resolved Hide resolved
src/TestFixture.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

Regarding the TODO I've opened #930. This will make it easier to perform tests at startup. Most of my comments are minor nitpicks.

src/TestFixture.cc Outdated Show resolved Hide resolved
src/TestFixture.cc Outdated Show resolved Hide resolved
@chapulina
Copy link
Contributor Author

This PR is ready for review. I recommend starting by reviewing #935 and #936 first, which are subsets of this PR and will make it shorter.

Once this is approved targeted at Fortress, I'm planning to retarget to Citadel, update as needed, merge, and forward-port up to Fortress.

@chapulina chapulina marked this pull request as ready for review July 29, 2021 03:11
@chapulina chapulina requested a review from arjo129 July 29, 2021 03:11
@chapulina chapulina dismissed ahcorde’s stale review July 29, 2021 21:36

All addressed

@chapulina
Copy link
Contributor Author

@mjcarroll , I merged from main and cleaned it up. It's ready for review 👍

Copy link
Contributor

@caguero caguero left a comment

Choose a reason for hiding this comment

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

It looks good to me. The only comment I have is that the example requires CMake 3.11 and Ubuntu Bionic ships with 3.10. We claim that Fortress is supported in Ubuntu Bionic. I think this is OK as it's one example and there are other alternative ways to install CMake.

@chapulina chapulina force-pushed the chapulina/6/fixture branch from 9963485 to a05623d Compare August 4, 2021 02:59
@chapulina chapulina changed the base branch from main to ign-gazebo3 August 4, 2021 03:00
@chapulina chapulina added 🏰 citadel Ignition Citadel and removed 🏯 fortress Ignition Fortress labels Aug 4, 2021
@chapulina
Copy link
Contributor Author

The PR was approved at 9963485 when targeting main (v6). I rebased onto ign-gazebo3 and updated the API to work there on a05623d.

I'm going to merge into ign-gazebo3 when CI comes back happy.

If anyone wants to use this with main before it's forward-ported, checkout 9963485.

@codecov
Copy link

codecov bot commented Aug 4, 2021

Codecov Report

Merging #926 (a05623d) into ign-gazebo3 (ad27b27) will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo3     #926      +/-   ##
===============================================
+ Coverage        77.80%   77.90%   +0.10%     
===============================================
  Files              220      221       +1     
  Lines            12579    12639      +60     
===============================================
+ Hits              9787     9847      +60     
  Misses            2792     2792              
Impacted Files Coverage Δ
include/ignition/gazebo/Util.hh 100.00% <100.00%> (ø)
src/TestFixture.cc 100.00% <100.00%> (ø)

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 ad27b27...a05623d. Read the comment docs.

@chapulina chapulina merged commit 8620011 into ign-gazebo3 Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants