[ros2bag test_record] Gets rid of time.sleep and move to using command.wait_for_output by jaisontj · Pull Request #525 · ros2/rosbag2

jaisontj · 2020-09-17T17:19:32Z

Tries to fix #454

Looks to me that the original problem described at #454 was probably addressed by #466
However, before #466, there was #462 which moved from using different temp directory per test to using one temp directory for all the tests - this PR also introduced the time.sleep within each test and also one within the cleanup block (which deletes the temp directory)

In this PR:

I've moved from using time.sleep within each tests to using bag_command.wait_for_output which essentially waits for the given timeout number or seconds for the expected output. This waiting needs to be done one way or another to give the process enough time to execute and print whatever it needs to.
I've also improved on the test conditions. For eg: test_qos_simple and test_incomplete_qos_profile were only testing if the bag_command.output was NOT of the format [ERROR] [ros2bag]. This causes a bug where the test will pass even if bag_command.output is empty ~ which can happen if the process is exited immediately i.e replace time.sleep(3) with a pass

time.sleep in tests Signed-off-by: Jaison Titus <jaisontj@amazon.com>

Signed-off-by: Jaison Titus <jaisontj@amazon.com>

to account for cases where wait_for_output is maybe called after the expected output is already printed. Signed-off-by: Jaison Titus <jaisontj@amazon.com>

jaisontj · 2020-09-18T17:59:50Z

Each of the tests can also be reduced to:

 def test_nonexistent_qos_profile(self):
        profile_path = PROFILE_PATH / 'foobar.yaml'
        output_path = Path(self.tmpdir.name) / 'ros2bag_test_nonexistent'
        arguments = ['record', '-a', '--qos-profile-overrides-path', profile_path.as_posix(),
                     '--output', output_path.as_posix()]
        expected_string_regex = re.compile(
            r'ros2 bag record: error: argument --qos-profile-overrides-path: can\'t open')
        with self.launch_bag_command(arguments=arguments) as bag_command:
            condition_result = bag_command.wait_for_output(
                condition=lambda output: expected_string_regex.search(output) is not None,
                timeout=5)
            assert condition_result, print('ros2bag CLI did not produce the expected output')
        bag_command.wait_for_shutdown(timeout=5)
        assert bag_command.terminated
        assert bag_command.exit_code != launch_testing.asserts.EXIT_OK

However, this can have an edge case where wait_for_command gets called after the expected string is printed out.

dabonnie

Were you able to run the tests repeatedly by any chance?

ros2bag/test/test_record_qos_profiles.py

tests. Signed-off-by: Jaison Titus <jaisontj@amazon.com>

Signed-off-by: Jaison Titus <jaisontj@amazon.com>

jaisontj · 2020-09-21T16:46:51Z

Were you able to run the tests repeatedly by any chance?

I've been testing on windows using: colcon test --packages-select ros2bag --retest-until-fail 25 --merge-install

jaisontj · 2020-09-21T16:48:57Z

Looks like the build failure on Rpr__rosbag2__ubuntu_focal_amd64 is unrelated to this PR as the associated tests have passed: http://build.ros2.org/job/Rpr__rosbag2__ubuntu_focal_amd64/206/console#console-section-5

jaisontj · 2020-09-21T18:53:12Z

Running on CI:

Linux
Linux-aarch64
macOS
Windows

jaisontj · 2020-09-21T20:27:18Z

CI On windows with --retest-until-fail 25

Windows

jaisontj · 2020-09-21T21:52:45Z

Do note that I was unable to get rid of the time.sleep here as this hack seems to be required.

emersonknapp

LGTM!

…d.wait_for_output (#525) * Uses bag_command.wait_for_output with expected string instead of time.sleep in tests Signed-off-by: Jaison Titus <jaisontj@amazon.com> * Fixes code style errors Signed-off-by: Jaison Titus <jaisontj@amazon.com> * Moves to asserting expected output match outside of the process context to account for cases where wait_for_output is maybe called after the expected output is already printed. Signed-off-by: Jaison Titus <jaisontj@amazon.com> * Defines timeout with variables and better error messages for failed tests. Signed-off-by: Jaison Titus <jaisontj@amazon.com> * Fixes flake8 errors Signed-off-by: Jaison Titus <jaisontj@amazon.com>

- test_record_qos_profiles failures is a known issue described in the #454 - To fix those tests need to backport #462, #466, #470, #472, #525 - Skipping them for a while Signed-off-by: Michael <michael.orlov@apex.ai>

- test_record_qos_profiles failures is a known issue described in the #454 - To fix those tests need to backport #462, #466, #470, #472, #525 - Skipping them for a while Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Fix for CI regression in TestRos2BagRecord - Replace get_actual_qos() to the qos_profile() as it was before #1335 For some reason on Foxy it doesn't work as expected. Probably due to the missing some underlying dependencies in other core packages. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Revert "Fix for CI regression in TestRos2BagRecord" This reverts commit f50f46a. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Skip test_record_qos_profiles on Windows since they are flaky - test_record_qos_profiles failures is a known issue described in the #454 - To fix those tests need to backport #462, #466, #470, #472, #525 - Skipping them for a while Signed-off-by: Michael Orlov <michael.orlov@apex.ai> --------- Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

jaisontj changed the title ~~[ros2bag test_record] Gets rid of time.sleep and move to using NamedTemporaryFile~~ [WIP][ros2bag test_record] Gets rid of time.sleep and move to using NamedTemporaryFile Sep 17, 2020

jaisontj marked this pull request as draft September 17, 2020 20:18

Uses bag_command.wait_for_output with expected string instead of

1568bb7

time.sleep in tests Signed-off-by: Jaison Titus <jaisontj@amazon.com>

jaisontj force-pushed the jaisontj/fix-454 branch from da88cbb to 1568bb7 Compare September 18, 2020 15:41

jaisontj added 2 commits September 18, 2020 10:29

Fixes code style errors

e2ce040

Signed-off-by: Jaison Titus <jaisontj@amazon.com>

Moves to asserting expected output match outside of the process context

b15c9fc

to account for cases where wait_for_output is maybe called after the expected output is already printed. Signed-off-by: Jaison Titus <jaisontj@amazon.com>

jaisontj force-pushed the jaisontj/fix-454 branch from 8ebcb5f to b15c9fc Compare September 18, 2020 17:51

jaisontj changed the title ~~[WIP][ros2bag test_record] Gets rid of time.sleep and move to using NamedTemporaryFile~~ [WIP][ros2bag test_record] Gets rid of time.sleep and move to using command.wait_for_output Sep 18, 2020

jaisontj marked this pull request as ready for review September 18, 2020 17:58

jaisontj requested review from dabonnie, emersonknapp and jikawa-az September 18, 2020 17:58

dabonnie suggested changes Sep 18, 2020

View reviewed changes

ros2bag/test/test_record_qos_profiles.py Outdated Show resolved Hide resolved

ros2bag/test/test_record_qos_profiles.py Outdated Show resolved Hide resolved

jaisontj added 2 commits September 21, 2020 16:36

Defines timeout with variables and better error messages for failed

a7881f5

tests. Signed-off-by: Jaison Titus <jaisontj@amazon.com>

Fixes flake8 errors

6e9d966

Signed-off-by: Jaison Titus <jaisontj@amazon.com>

jaisontj requested a review from dabonnie September 21, 2020 16:41

jaisontj changed the title ~~[WIP][ros2bag test_record] Gets rid of time.sleep and move to using command.wait_for_output~~ [ros2bag test_record] Gets rid of time.sleep and move to using command.wait_for_output Sep 21, 2020

dabonnie approved these changes Sep 21, 2020

View reviewed changes

emersonknapp approved these changes Sep 22, 2020

View reviewed changes

jikawa-az approved these changes Sep 22, 2020

View reviewed changes

jaisontj mentioned this pull request Sep 22, 2020

ros2bag QOS test uses several sleeps on windows #454

Closed

jaisontj merged commit 9b72b4a into ros2:master Sep 22, 2020

jaisontj deleted the jaisontj/fix-454 branch September 22, 2020 23:53

MichaelOrlov mentioned this pull request Oct 25, 2022

Check & create database dirs before writing bags #1133

Merged

MichaelOrlov mentioned this pull request May 25, 2023

[foxy] Disable test_record_qos_profiles tests on Windows #1349

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ros2bag test_record] Gets rid of time.sleep and move to using command.wait_for_output#525

[ros2bag test_record] Gets rid of time.sleep and move to using command.wait_for_output#525
jaisontj merged 5 commits intoros2:masterfrom
aws-ros-dev:jaisontj/fix-454

jaisontj commented Sep 17, 2020 •

edited

Loading

Uh oh!

jaisontj commented Sep 18, 2020

Uh oh!

dabonnie left a comment

Uh oh!

Uh oh!

Uh oh!

jaisontj commented Sep 21, 2020

Uh oh!

jaisontj commented Sep 21, 2020

Uh oh!

jaisontj commented Sep 21, 2020

Uh oh!

jaisontj commented Sep 21, 2020

Uh oh!

jaisontj commented Sep 21, 2020

Uh oh!

emersonknapp left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

jaisontj commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaisontj commented Sep 18, 2020

Uh oh!

dabonnie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jaisontj commented Sep 21, 2020

Uh oh!

jaisontj commented Sep 21, 2020

Uh oh!

jaisontj commented Sep 21, 2020

Uh oh!

jaisontj commented Sep 21, 2020

Uh oh!

jaisontj commented Sep 21, 2020

Uh oh!

emersonknapp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jaisontj commented Sep 17, 2020 •

edited

Loading