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.

Sign up for GitHub

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

Jump to bottom

Fix launch warnings #421

Merged
merged 3 commits into from
May 14, 2020
Merged

Fix launch warnings #421

merged 3 commits into from
May 14, 2020

Conversation

clalancette
Copy link
Contributor

@clalancette clalancette commented May 13, 2020 •
edited
Loading

This PR fixes a bunch of warnings in launch.

  1. pytest on Focal and Bionic complains that:
  Warning: direct construction of LaunchTestModule has been deprecated, please use LaunchTestModule.from_parent
  1. Launch itself complains that:
  Warning: Automatically adding attributes like self.proc_info to the test class will be deprecated in a future release.  Instead, add proc_info to the test method argument list to access the test object you need

(this is fixed in several tests)
3. pytest on Bionic complains that:

  Warning: The 'junit_family' default value will change to 'xunit2' in pytest 6.0.
  Add 'junit_family=xunit1' to your pytest.ini file to keep the current format in future versions of pytest and silence this warning.

With all of these fixes in place, I get green CI again.

@dirk-thomas
Copy link
Member

This overlaps with #420.

@clalancette
Copy link
Contributor Author

This overlaps with #420.

Yep, pointed out in the initial description. Only the first one does.

dirk-thomas
dirk-thomas reviewed May 13, 2020
View reviewed changes
launch_testing/pytest.ini Outdated
@@ -3,3 +3,4 @@
testpaths = test
# Add arguments for launch tests
addopts = --launch-args dut_arg:=test
junit_family=xunit1
Copy link
Member

Choose a reason for hiding this comment

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

This is not compatible with ci.ros2.org: https://github.com/ros2/ci/blob/a7c41fbce3f1acee2a98a0e2865a8c9a0fb0db8a/ros2_batch_job/__main__.py#L381

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks for the feeback. Will set to xunit2.

Copy link
Member

Choose a reason for hiding this comment

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

CI already sets the flag automatically. So why do you want to set it explicitly (which would be the only package which does that)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because otherwise there's a warning from CI:

  Warning: The 'junit_family' default value will change to 'xunit2' in pytest 6.0.
  Add 'junit_family=xunit1' to your pytest.ini file to keep the current format in future versions of pytest and silence this warning.

I think this needs a custom pytest.ini to add options, but I really don't know. @ivanpauno @hidmic any thoughts here?

Copy link
Member

Choose a reason for hiding this comment

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

I think that if the warning is silenced by explicitly specifying the junit_familiy here and CI passes, it's ok this approach (besides that CI already sets the junit family).

Copy link
Member

@dirk-thomas dirk-thomas May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Because otherwise there's a warning from CI:

Can you please reference the build where you get this warning. On ci.ros2.org this shouldn't happen because of ros2/ci#406.

Copy link
Member

Choose a reason for hiding this comment

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

I think that if the warning is silenced by explicitly specifying the junit_familiy here and CI passes, it's ok this approach (besides that CI already sets the junit family).

The question is why it is necessary since the CI logic tries to workaround this already. And if it is necessary I would assume it is the same for all packages and then should also be addressed for all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right. This doesn't happen in CI because of the override. I only saw it locally, which is why we need it.

Copy link
Member

Choose a reason for hiding this comment

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

I only saw it locally, which is why we need it.

Then please apply it in a separate pull request to all packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then please apply it in a separate pull request to all packages.

I really don't see why. This improves it for this package; adding it to other packages to fix it there is orthogonal.

ivanpauno
ivanpauno approved these changes May 13, 2020
View reviewed changes
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGMT!

@clalancette
Switch to from_parent to remove deprecation warning.
73737cc 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Stop using implicit variables in example testing.
3289ec8 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Set junit_family to xunit2 in pytest.ini
f3d5e86 
This avoids a warning like:

Warning: The 'junit_family' default value will change to 'xunit2' in pytest 6.0.
  Add 'junit_family=xunit1' to your pytest.ini file to keep the current format in future versions of pytest and silence this warning.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
hidmic
hidmic approved these changes May 13, 2020
View reviewed changes
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.

LGTM too

@clalancette
Copy link
Contributor Author

clalancette commented May 14, 2020 •
edited
Loading

CI:

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

@clalancette clalancette mentioned this pull request May 14, 2020
Merged
@clalancette
Copy link
Contributor Author

All right, green CI and approval on both this and ros2/launch_ros#150. I'm going to merge these; we can follow up with more pytest warning fixes in a follow-up.

@clalancette clalancette merged commit 3a6bde7 into master May 14, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix-launch-warnings branch May 14, 2020 16:15
@dirk-thomas dirk-thomas mentioned this pull request Jun 16, 2020
Merged
@sloretz sloretz mentioned this pull request Sep 8, 2020
Merged
sloretz added a commit that referenced this pull request Sep 8, 2020
@sloretz @rotu
@clalancette @dirk-thomas
[Eloquent backport] stop using constructors deprecated in pytest 5.4 (#…
b750eda 
…391) Switch to from_parent (partial #421) avoid deprecation warning, use from_parent (#402) (#459)

* stop using constructors deprecated in pytest 5.4 (#391)

Signed-off-by: Dan Rose <dan@digilabs.io>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Switch to from_parent to remove deprecation warning.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* avoid deprecation warning, use from_parent (#402)

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Co-authored-by: Dan Rose <rotu@users.noreply.github.com>
Co-authored-by: Chris Lalancette <clalancette@openrobotics.org>
Co-authored-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirk-thomas dirk-thomas dirk-thomas left review comments

@ivanpauno ivanpauno ivanpauno approved these changes

@hidmic hidmic hidmic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@clalancette @dirk-thomas @hidmic @ivanpauno