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

Run ign-cmake's copy of check_test_ran #168

Merged
merged 2 commits into from
May 3, 2021

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented May 1, 2021

Summary

Ignition CMake has been running check_test_ran.py scripts contained in each Ignition library. But these scripts are all very similar, if not exactly the same. It's much more convenient to maintain a single copy of this script that all libraries can use.

  • Update check_test_ran to match the one on ign-gazebo so that it prints meaningful test names.
  • Install the tools directory alongside benchmark and coverage.
  • Note that this also installs tools/doc_check.sh, which is not used yet, but once it's installed, it could be used from CMake instead of downloading it from GitHub, see Add documentation check with doxygen gazebo-tooling/action-gz-ci#30.
  • Remove the tools/code_check.sh script, which I think doesn't have any use currently.

I don't believe any changes are needed in https://github.com/ignition-release/ign-cmake2-release to install these files, because benchmark and coverage are already being installed correctly.

Test it

Install this branch with this PR gazebosim/gz-math#211, which removes that library's own copy of the script.

Checklist

  • Signed all commits for DCO
  • Added tests - no 😢 just manual testing with ign-math ☝️
  • 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 scpeters May 1, 2021 01:35
@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels May 1, 2021
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

This looks good to me. My only thought is a bike-shed about installing to scripts instead of tools, but this is not important

@chapulina chapulina mentioned this pull request May 3, 2021
7 tasks
Copy link
Contributor

@j-rivero j-rivero 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 cleaning up the whole mess. I don't see any problem with the changes in the code.

@chapulina chapulina merged commit d8cd96c into ign-cmake2 May 3, 2021
@chapulina chapulina deleted the chapulina/2/check_test_ran branch May 3, 2021 16:45
adlarkin added a commit that referenced this pull request May 3, 2021
Signed-off-by: Ashton Larkin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants