Add timeout for ReadyToTest action

[deepanshubansal01]

Related to #582

Signed-off-by: deepanshu [email protected]

[adityapande-1995]

Can we have a test that uses multiple runners or Launch descriptions ? Probably like a parameterized test here ?

launch/launch_testing/test/launch_testing/test_launch_test_runner_validation.py

Line 157 in 69964b7

@launch_testing.parametrize('my_param', [1, 2, 3])

[deepanshubansal01]

Reference

I can try that but is this test for checking if we can we have multiple runners/launch descriptions or for testing the timeout thing?
Also for the params I am not sure if they are the same as cli optional arguments parsed using argsparse lib.

[adityapande-1995]

Reference

I can try that but is this test for checking if we can we have multiple runners/launch descriptions or for testing the timeout thing? Also for the params I am not sure if they are the same as cli optional arguments parsed using argsparse lib.

Yup I meant if launch allows multiple runners at all, we should have a timeout test in that situation.

[deepanshubansal01]

Reference

I can try that but is this test for checking if we can we have multiple runners/launch descriptions or for testing the timeout thing? Also for the params I am not sure if they are the same as cli optional arguments parsed using argsparse lib.

Yup I meant if launch allows multiple runners at all, we should have a timeout test in that situation.

Based on our discussion offline we don't need this now.

[jacobperron]

The new option looks good to me 👍

[deepanshubansal01]

@ros-pull-request-builder retest this please

[ivanpauno]

IMO, it would be nice if the timeout is somehow specified in the launch file, instead of being an option for the as a command line argument.

Is there any alternative to make that happen?

[ivanpauno]

nit (in this way, the diff will not show one deleted line next time):

Suggested change

	timeout=args.timeout
	timeout=args.timeout,

[deepanshubansal01]

Thanks!

[ivanpauno]

IMO, it would be nice if the timeout is somehow specified in the launch file, instead of being an option for the as a command line argument.

How would the timeout parameter being added here being specified in practice?
i.e. where would the users specify it? (users are not usually running launch_testing manually)

[deepanshubansal01]

IMO, it would be nice if the timeout is somehow specified in the launch file, instead of being an option for the as a command line argument.

Is there any alternative to make that happen?

I tried making timeout configurable as part of launch file itself and having timeout as an argument to the ReadyToTest action. However, this is not possible with the current code base(based on my findings). The _run_test thread which checks for ReadyToTest Action timeout is started in the _RunnerWorker class constructor and the ReadyToTest action itself is instantiated after the _run_test thread has been started which makes it futile to access the timeout attribute from ReadyToTest action class since the _run_test thread has already been started.

The _run_test thread is started here:

launch/launch_testing/launch_testing/test_runner.py

Lines 60 to 64 in 074cd29

	self._test_tr = threading.Thread(
	target=self._run_test,
	name='test_runner_thread',
	daemon=True
	)

And the ReadyToTest object is instantiated here:

launch/launch_testing/launch_testing/test_runner.py

Lines 73 to 75 in 074cd29

	test_ld, test_context = self._test_run.normalized_test_description(
	ready_fn=lambda: self._processes_launched.set()
	)

[deepanshubansal01]

IMO, it would be nice if the timeout is somehow specified in the launch file, instead of being an option for the as a command line argument.

How would the timeout parameter being added here being specified in practice? i.e. where would the users specify it? (users are not usually running launch_testing manually)

I am not sure about this but right now the timeout can be passed through cli and manually to the LaunchTestRunner class. If the use case is not through cli then we might have to do something else, passing it in launch file itself makes much more sense I guess as part of ReadyToTest action but I am not sure how can we do it with the current design.

[ivanpauno]

passing it in launch file itself makes much more sense I guess as part of ReadyToTest action but I am not sure how can we do it with the current design.

Ok, I will take a look tomorrow and comment if I have any ideas.

[jacobperron]

I couldn't think of way to configure the timeout in the launch file (though I agree it would be nicer).

[ivanpauno]

Ideally, I think that a decorator would be a nice way to specify it:

@launch_testing.runner_processes_wait_timeout(5.3)
def generate_test_description(arg_param):

To have something like that, the decorator would have to add a hidden attribute to the decorated function (e.g. generate_test_description.__runner_processes_wait_timeout).
The loader could read that attribute, similar to how parametrized launch descriptions are handled.
Each TestRun could have the timeout as a property.
And then the runner would use self._test_run.runner_processes_wait_timeout instead of 15 here.

@deepanshubansal01 @jacobperron does that make sense to you?

[ivanpauno]

@hidmic maybe you can provide more feedback of how to implement this

[deepanshubansal01]

@hidmic maybe you can provide more feedback of how to implement this

@ivanpauno should I proceed with the implementation that you suggested or wait for @hidmic inputs.

[ivanpauno]

@ivanpauno should I proceed with the implementation that you suggested or wait for @hidmic inputs.

I think it's fine to proceed with the implementation.

[deepanshubansal01]

Closing the PR now, the feature will be implemented in this PR now #625