Support LaunchService injection into pre-shutdown tests.#308
Conversation
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
I was thinking about simply looping, yes. We could also instrument a decorator for our tests that repeats it until it passes or timeout is reached, akin to |
|
CI up to
|
|
Alright, all green. Going in! @dirk-thomas feel free to comment async. |
| self._test_run.bind( | ||
| self._test_run.pre_shutdown_tests, | ||
| injected_attributes={ | ||
| 'launch_service': self._launch_service, |
There was a problem hiding this comment.
@hidmic Not directly related to this PR, but do you have any guidance for a process for deprecating 'injected_attributes' from launch_testing? Something like 'put a warning in the next release when someone uses an injected attribute' then in the release after that remove injected attributes?
I think injected_args are better and less confusing in just about every way. They still require the user to know ahead of time what 'magic words' can be added as arguments to a test function, but beyond that they're way less mysterious
There was a problem hiding this comment.
Hmm, you could wrap them using a property that can generate a warning once if these are used.
There was a problem hiding this comment.
@hidmic Yeah - I know how to do the implementation side - my question is more about the administrative side. Do we need a full release of "WARNING THIS WILL BE DEPRECATED" or is it sufficient to add the warning, them actually deprecate once all of OSRF's stuff is updated?
There was a problem hiding this comment.
Oops, forgot to reply. And sorry for the noise, I clearly half read what you wrote before.
I actually don't know what's the deprecation cycle for Python APIs here. What you describe makes sense to me. We'd be looking forward to issuing a DeprecationWarning in Eloquent and removing the feature entirely on F-Turtle. @dirk-thomas any thoughts?
There was a problem hiding this comment.
If a deprecation cycle is technically feasible it is certainly preferred to give users time to migrate.
| ) -> Optional[LaunchDescription]: | ||
| typed_event = cast(ShutdownProcess, context.locals.event) | ||
| if not typed_event.process_matcher(self): | ||
| # this event whas not intended for this process |
There was a problem hiding this comment.
Spelling.
Also that this is used twice but references different entities is a bit hard to read.
Precisely what the title says. Closes #295.