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

Enable launch test discovery in pytest #312

Merged
merged 13 commits into from
Aug 23, 2019
Merged

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Aug 16, 2019

Closes #237. This pull request allows for launch tests discovery and execution via pytest. It also sets up the scene for downstream packages i.e. launch_testing_ros to customize this.

Note this is not a tight integration between launch_testing and pytest but simply a mechanism for the latter to be able to execute launch tests.

Depends on #308.

hidmic added 3 commits August 20, 2019 14:53
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic force-pushed the hidmic/launch-tests-in-python branch from 1af785b to 94922bc Compare August 20, 2019 17:54
@hidmic
Copy link
Contributor Author

hidmic commented Aug 20, 2019

CI up to launch_testing, launch_testing_ros and test_communication for extra safety:

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

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.

I left some minimal comments.
I surfed in pytest plugin documentation and I don't fully understand the patch. But I don't have much more to add if it's working.

def find_launch_test_entrypoint(path):
try:
return getattr(path.pyimport(), 'generate_test_description', None)
except SyntaxError:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if that's the only possible exception, I would catch everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A SyntaxError is the only error I'm expecting. I'd rather not catch (and hide) unexpected errors e.g. some exception as a result of code being executed at the module level.

Copy link
Member

Choose a reason for hiding this comment

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

launch_testing/test/launch_testing/examples/README.md Outdated Show resolved Hide resolved
@ivanpauno
Copy link
Member

Extra comment, instead of rostest files needing a pytest mark and launchtest files just needing a generate_test_description function in the file, why not requiring a mark in them too? Something like, @pytest.mark.launchtest.
I prefer the difference being more explicit (than one explicit and the other implicit).

Also, aren't files with @pytest.mark.rostest mark being collected by both hooks now?

hidmic added 7 commits August 21, 2019 12:29
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
…unch_testing plugin.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Aug 21, 2019

Extra comment, instead of rostest files needing a pytest mark and launchtest files just needing a generate_test_description function in the file, why not requiring a mark in them too? Something like, @pytest.mark.launchtest.
I prefer the difference being more explicit (than one explicit and the other implicit).

Right now, any module in the search path that has a generate_test_description() is taken as a launch test and thus any mark was redundant in the pure launch test case. But I see the value in being explicit in both cases. Check 4491759.

Also, aren't files with @pytest.mark.rostest mark being collected by both hooks now?

Nope, this hook collects them appropriately once.

@hidmic
Copy link
Contributor Author

hidmic commented Aug 21, 2019

I had to fix other failing tests, due to examples not being installed and pytest compulsively re-writing asserts within launch_testing. @ivanpauno PTAL.

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.

LGTM with green CI

def __iter__(self):
return iter(self.__calls)
yield partial, partial_args
setattr(_wrapped, '__parametrized__', True)
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't be:

_wrapped.__parametrized__ = True

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it can be. Done in 0705777.

"""
A module providing exit code assertions.

PYTEST_DONT_REWRITE
Copy link
Member

Choose a reason for hiding this comment

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

Maybe, add a comment/link explaining why this is needed (e.g.: https://docs.pytest.org/en/latest/assert.html#assertion-introspection-details).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done in c7e6f80.

hidmic added 2 commits August 22, 2019 15:14
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Aug 23, 2019

Alright, all green! Going in.

@hidmic hidmic merged commit 5d6a87b into master Aug 23, 2019
@delete-merged-branch delete-merged-branch bot deleted the hidmic/launch-tests-in-python branch August 23, 2019 16:24
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.

Hook up launch tests in pure Python packages
4 participants