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

Use test fixtures to create SROS artifacts #522

Merged
merged 2 commits into from Aug 7, 2023
Merged

Conversation

cottsay
Copy link
Member

@cottsay cottsay commented Aug 5, 2023

The current implementation creates SROS artifacts during the configure phase. This is undesirable for a variety of reasons, such as:

  • The ros2 binary is invoked during build, meaning that a regression in that tool will break the entire CI build instead of appearing as a test failure.
  • Some of the environment is captured during the artifact generation and it may not be the same when the tests are invoked. Namely, changes to ROS_DOMAIN_ID between build and test will break.

The test fixtures work similarly to tests themselves, and are automatically run if any tests requiring them are enabled. If the fixture fails, the dependent tests are not attempted.

The current implementation creates SROS artifacts during the configure
phase. This is undesirable for at least a variety of reasons, such as:
* The ros2 binary is invoked during build, meaning that a regression in
  that tool will break the entire CI build instead of appearing as a
  test failure.
* Some of the environment is captured during the artifact generation
  and it may not be the same when the tests are invoked. Namely,
  changes to ROS_DOMAIN_ID between build and test will break.

The test fixtures work similarly to tests themselves, and are
automatically run if any tests requiring them are enabled. If the
fixture fails, the dependent tests are not attempted.

Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay cottsay added the enhancement New feature or request label Aug 5, 2023
@cottsay
Copy link
Member Author

cottsay commented Aug 5, 2023

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

Prototype build with this change and parallelized tests: Build Status

test_security/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Overall, this looks like a great cleanup. I've left a couple of things to think about.

test_security/CMakeLists.txt Outdated Show resolved Hide resolved
Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay
Copy link
Member Author

cottsay commented Aug 7, 2023

Fresh CI:

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

Copy link
Contributor

@clalancette clalancette 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 with green CI.

@cottsay cottsay merged commit d98fcb9 into rolling Aug 7, 2023
3 checks passed
@delete-merged-branch delete-merged-branch bot deleted the cottsay/test-fixtures branch August 7, 2023 22:08
@cottsay
Copy link
Member Author

cottsay commented Aug 7, 2023

Thanks for the review.

In order to really leverage this on ci.ros2.org, we'll probably want to backport it to Iron and Humble. CMake 3.7 is an acceptable limitation for both of those platforms from what I can tell, and this shouldn't have any affect on payload.

Do you see any reason not to backport?

@clalancette
Copy link
Contributor

Do you see any reason not to backport?

No, I think that is totally fine.

@cottsay
Copy link
Member Author

cottsay commented Aug 8, 2023

@Mergifyio backport iron humble

@mergify
Copy link

mergify bot commented Aug 8, 2023

backport iron humble

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Aug 8, 2023
The current implementation creates SROS artifacts during the configure
phase. This is undesirable for a variety of reasons, such as:
* The ros2 binary is invoked during build, meaning that a regression in
  that tool will break the entire CI build instead of appearing as a
  test failure.
* Some of the environment is captured during the artifact generation
  and it may not be the same when the tests are invoked. Namely,
  changes to ROS_DOMAIN_ID between build and test will break.

The test fixtures work similarly to tests themselves, and are
automatically run if any tests requiring them are enabled. If the
fixture fails, the dependent tests are not attempted.

(cherry picked from commit d98fcb9)
mergify bot pushed a commit that referenced this pull request Aug 8, 2023
The current implementation creates SROS artifacts during the configure
phase. This is undesirable for a variety of reasons, such as:
* The ros2 binary is invoked during build, meaning that a regression in
  that tool will break the entire CI build instead of appearing as a
  test failure.
* Some of the environment is captured during the artifact generation
  and it may not be the same when the tests are invoked. Namely,
  changes to ROS_DOMAIN_ID between build and test will break.

The test fixtures work similarly to tests themselves, and are
automatically run if any tests requiring them are enabled. If the
fixture fails, the dependent tests are not attempted.

(cherry picked from commit d98fcb9)
@clalancette
Copy link
Contributor

@cottsay It looks like this may have lead to a regression in the nightly repeated jobs: https://ci.ros2.org/view/nightly/job/nightly_linux-aarch64_repeated/2468/testReport/(root)/projectroot/remove_missing_key/ .

Can you please take a look?

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

Successfully merging this pull request may close these issues.

None yet

2 participants