Skip to content

Increase coverage with a graveyard behavior test and unmanaged instance test #159

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

Merged
merged 4 commits into from
Jun 11, 2020

Conversation

brawner
Copy link
Contributor

@brawner brawner commented Jun 5, 2020

Reviving symbols from the graveyard is currently not covered in unit tests, at least on linux and Windows. It may be covered on OSX. I think this is mostly due to the fact that RTLD_LOCAL is the default for dlopen() on linux according to https://linux.die.net/man/3/dlopen. On OSX it appears the default is [RTLD_GLOBAL] (http://mirror.informatimago.com/next/developer.apple.com/documentation/Darwin/Reference/ManPages/man3/dlopen.3.html). This utility does not exist on Windows, so this feature will remain unused/untested on Windows.

The added test, just forces it to be loaded using RTLD_GLOBAL before it is loaded by class_loader. Because I use dlopen directly, I have separated the namespace of the test to force the static initialization of ClassLoader to be isolated from other tests. Likewise, I use a new test plugin library so that it is isolated from other tests.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Signed-off-by: Stephen Brawner brawner@gmail.com

@brawner brawner force-pushed the brawner/graveyard-test branch from 714fd89 to e760503 Compare June 5, 2020 01:10
@brawner brawner changed the title Unit test for graveyard behavior with dlopen RTLD_GLOBAL Increase coverage with a graveyard behavior test and unmanaged instance test Jun 5, 2020
@brawner brawner force-pushed the brawner/graveyard-test branch 3 times, most recently from fae51f1 to e58ee07 Compare June 5, 2020 01:17
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner brawner force-pushed the brawner/graveyard-test branch from e58ee07 to 3e9f726 Compare June 5, 2020 01:25
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

LGTM, just review the license and the comment

@@ -0,0 +1,76 @@
/*
* Copyright (c) 2012, Willow Garage, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

review the copyright

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a second copyright line and line about what was modified at the bottom of the license.

Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner brawner force-pushed the brawner/graveyard-test branch from 7235576 to 1173c9c Compare June 8, 2020 18:07
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

Overall LGTM

Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
Copy link
Contributor Author

brawner commented Jun 10, 2020

Running tests again:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
Copy link
Contributor Author

brawner commented Jun 10, 2020

Just updating the QD

@@ -114,7 +114,9 @@ This includes:

Changes are required to make a best effort to keep or increase coverage before being accepted, but decreases are allowed if properly justified and accepted by reviewers.

Current coverage statistics can be viewed [here](https://ci.ros2.org/job/ci_linux_coverage/85/cobertura/src_ros_class_loader_include_class_loader/) and [here](https://ci.ros2.org/job/ci_linux_coverage/85/cobertura/src_ros_class_loader_include_class_loader/). This package does not yet meet the 95% coverage guideline, but it is currently above 90%.
This package has testing coverage of at least 95%.
Current coverage statistics can be viewed [here](https://ci.ros2.org/job/nightly_linux_coverage/lastSuccessfulBuild/cobertura/).
Copy link
Contributor

Choose a reason for hiding this comment

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

The link would be this, right?

Suggested change
Current coverage statistics can be viewed [here](https://ci.ros2.org/job/nightly_linux_coverage/lastSuccessfulBuild/cobertura/).
Current coverage statistics can be viewed [here](https://ci.ros2.org/job/nightly_linux_coverage/lastSuccessfulBuild/cobertura/src_ros_class_loader_src/).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've started just linking to the full cobertura report because the statistics for a package has to be composed from multiple lines. In this case, there is coverage in the include and src directories.

I'm open to better approaches for this sort of thing though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm, isn't the full coverage reported under its package folder? The fact that the coverage is tested with more packages is stated in the developer guide, IMO it should be enough.

Copy link
Contributor Author

@brawner brawner Jun 11, 2020

Choose a reason for hiding this comment

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

Since there are functions in the include directory, coverage for class_loader should be composed from:

https://ci.ros2.org/job/nightly_linux_coverage/lastSuccessfulBuild/cobertura/src_ros_class_loader_include_class_loader/
and
https://ci.ros2.org/job/nightly_linux_coverage/lastSuccessfulBuild/cobertura/src_ros_class_loader_src/

As described in the developer guide, the coverage statistics should include src.*.<repository_name>.<package_name>.* and build/.<repository_name>.* (if it exists). Depending on the package that may be a lot of links to include in a single paragraph. Seeing as the number of links might grow or shrink as the package grows or shrinks, I figured it would be a lot easier to just link to the main page.

@nuclearsandwich
Copy link
Member

Merging this PR based on the approvals above. I haven't had the time to review myself but I don't feel the need to based on the trust I have in the contributors and reviewers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants