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

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

Merged
merged 5 commits into from
Sep 22, 2020

Conversation

jaisontj
Copy link
Contributor

@jaisontj jaisontj commented Sep 17, 2020

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

@jaisontj 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 jaisontj marked this pull request as draft September 17, 2020 20:18
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 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 jaisontj marked this pull request as ready for review September 18, 2020 17:58
@jaisontj
Copy link
Contributor Author

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.

Copy link
Contributor

@dabonnie dabonnie left a comment

Choose a reason for hiding this comment

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

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

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

Signed-off-by: Jaison Titus <jaisontj@amazon.com>
Signed-off-by: Jaison Titus <jaisontj@amazon.com>
@jaisontj jaisontj requested a review from dabonnie September 21, 2020 16:41
@jaisontj
Copy link
Contributor Author

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
Copy link
Contributor Author

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 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
@jaisontj
Copy link
Contributor Author

Running on CI:

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

@jaisontj
Copy link
Contributor Author

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

  • Windows Build Status

@jaisontj
Copy link
Contributor Author

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

Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

LGTM!

@jaisontj jaisontj merged commit 9b72b4a into ros2:master Sep 22, 2020
@jaisontj jaisontj deleted the jaisontj/fix-454 branch September 22, 2020 23:53
emersonknapp pushed a commit that referenced this pull request Feb 2, 2021
…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>
emersonknapp pushed a commit that referenced this pull request Feb 17, 2021
…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>
MichaelOrlov added a commit that referenced this pull request May 25, 2023
- 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>
MichaelOrlov added a commit that referenced this pull request May 25, 2023
- 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>
MichaelOrlov added a commit that referenced this pull request May 25, 2023
* 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>
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.

ros2bag QOS test uses several sleeps on windows
4 participants