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.

Sign up for GitHub

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

Jump to bottom

Revert "Revert "[execute_process] emulate_tty configurable and defaults to true"" #277

Merged
merged 9 commits into from
Jul 24, 2019
Merged

Revert "Revert "[execute_process] emulate_tty configurable and defaults to true"" #277

merged 9 commits into from
Jul 24, 2019

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Jul 18, 2019

This is a refactor of #265 so that it doesn't break our other tests.

Detecting it in the first place was my fault because I narrowed our CI testing too much and so didn't exercise the failing tests.

Reverts #276

@wjwwood wjwwood added the bug Something isn't working label Jul 18, 2019
@wjwwood wjwwood self-assigned this Jul 18, 2019
@dirk-thomas
Copy link
Member

If want a fast CI response for this change you can try the new CI overlay job. It uses all unchanged packages from last nights build and only builds the specified changes: see http://build.ros2.org/view/Eci/job/Eci__overlay_ubuntu_bionic_amd64/21/parameters/ which finished in less than 15 min.

@wjwwood wjwwood added the in review Waiting for review (Kanban column) label Jul 19, 2019
@wjwwood
Copy link
Member Author

wjwwood commented Jul 19, 2019

Ok, I found the issue, it was the ANSI escape sequences (printed by the console logger to reset terminal colors) were being captured when emulate tty was enabled, which broke output comparison. I updated launch testing to strip these by default (with an option to not strip them in case someone needed that feature later).

@pbaughman can you look at 12c0563? I moved this around because I noticed it and thought it might be a race where the event handlers could miss some events from processes included in the launch description being tested. I don't think it was the issue, but while debugging I considered it might be an issue. I can revert it if needed, but I think the new code is more correct.

@wjwwood
Copy link
Member Author

wjwwood commented Jul 19, 2019 •
edited
Loading

Here's full CI:

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

@dirk-thomas
Copy link
Member

dirk-thomas commented Jul 19, 2019 •
edited
Loading

A CI build on build.ros2.org shows the same test failures as previously (not sure if I configured something incorrectly?): http://build.ros2.org/view/Eci/job/Eci__overlay_ubuntu_bionic_amd64/25/testReport/

@wjwwood
Copy link
Member Author

wjwwood commented Jul 19, 2019

Configuration looks ok, not sure. It definitely fixed it in my local vm and I pushed everything. The job used the right commit of ros2/launch (afaict).

The failing test names and captured output are kind of not very useful though.

@wjwwood
Copy link
Member Author

wjwwood commented Jul 19, 2019

Is it possible that the overlaid demo_nodes_cpp is using the launch from the underlay?

@dirk-thomas
Copy link
Member

Is it possible that the overlaid demo_nodes_cpp is using the launch from the underlay?

It shouldn't. But I wouldn't go as far as to say that it is impossible 😉

@wjwwood
Copy link
Member Author

wjwwood commented Jul 19, 2019

It's possible this fix isn't perfect, let's see what ci.ros2.org says, and I'll investigate with CI jobs containing print statements in launch if needed.

@wjwwood
Copy link
Member Author

wjwwood commented Jul 19, 2019

Looks like it is failing on ci.ros2.org too (just reading the output so far), I guess I'll have to have another look. It might be the \n versus \r\n, but I don't know why my VM works though.

@wjwwood
Copy link
Member Author

wjwwood commented Jul 20, 2019

Ok, all the tests in demo_nodes_cpp were fixed, but none of those used the regex feature :|. So now I changed how newline replacement works because when emulating a tty, \r\n is used as the line ending rather than just \n or os.linesep as it is in files. I updated the logic to handle this. We do the same in osrf_pycommon: https://github.com/osrf/osrf_pycommon/blob/8ba78744d9df7de70d824b4d76c11afe9e81ccd1/osrf_pycommon/process_utils/execute_process_pty.py#L80-L81

But it's still failing on the build.ros2.org ci: http://build.ros2.org/view/Eci/job/Eci__overlay_ubuntu_bionic_amd64/26/console

I'm not sure what's happening just yet.

@wjwwood
Copy link
Member Author

wjwwood commented Jul 20, 2019

Running a smaller set of CI on ci.ros2.org, because I think it's possible that nothing is fixed on build.ros.org (demo_nodes_cpp nor intra_process_demo) but that before demo_nodes_cpp was fixed on ci.ros2.org and only intra_process_demo and other regex based tests were still failing:

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

@wjwwood
Copy link
Member Author

wjwwood commented Jul 20, 2019

Yeah, looks like the build.ros2.org CI job is not catching the fix some how, the demo_nodes_cpp issues are fixed in https://ci.ros2.org/job/ci_linux/7626/testReport/ (from the first set of CI I started on ci.ros2.org).

@pbaughman
Copy link
Contributor

@pbaughman can you look at 12c0563? I moved this around because I noticed it and thought it might be a race where the event handlers could miss some events from processes included in the launch description being tested. I don't think it was the issue, but while debugging I considered it might be an issue. I can revert it if needed, but I think the new code is more correct.

@wjwwood I never considered that the order of event handlers and execute processes could matter but yeah - you're right. In a perfect world, I would hope it wouldn't matter but in the real world, yours is almost certainly more correct.

@wjwwood
Copy link
Member Author

wjwwood commented Jul 22, 2019

I need to still look at this failure (last one):

https://ci.ros2.org/job/ci_linux/7629/testReport/launch.test.launch.actions/test_include_launch_description/test_include_launch_description_launch_arguments/

But it's failing on nightly too:

https://ci.ros2.org/view/nightly/job/nightly_linux_release/1245/testReport/launch.test.launch.actions/test_include_launch_description/test_include_launch_description_launch_arguments/

So, I'd say this is up for review.

@wjwwood
Copy link
Member Author

wjwwood commented Jul 22, 2019

CI:

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

@wjwwood
Copy link
Member Author

wjwwood commented Jul 22, 2019

@stonier I refactored two parts of the original pull request, just FYI in case you care to have a look.

@wjwwood
Copy link
Member Author

wjwwood commented Jul 23, 2019

9b413c1 also fixes the test failure that is also on master for the nightly's.

@stonier
Copy link
Contributor

stonier commented Jul 23, 2019

Looks good to me - nicer way of checking the boolean.

@wjwwood
Copy link
Member Author

wjwwood commented Jul 23, 2019

I'm looking into https://ci.ros2.org/job/ci_linux/7635/testReport/(root)/projectroot/test_logging_output_format/ now, which also seems to be related to this change.

@wjwwood wjwwood mentioned this pull request Jul 23, 2019
Merged
hidmic
hidmic approved these changes Jul 23, 2019
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM as long as CI's happy

@wjwwood
Copy link
Member Author

wjwwood commented Jul 24, 2019

So this last issue has become a bit crazy. I'm still working on it, but I'm going to change the default of emulate_tty to be False (current behavior), and make a new pull request to change it to be True by default along with any fixes needed to make the last kind of failing test pass.

@wjwwood wjwwood force-pushed the revert-276-revert-265-emulate_tty branch from b58c950 to 2b977c0 Compare July 24, 2019 20:08
@wjwwood
Copy link
Member Author

wjwwood commented Jul 24, 2019

CI:

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

@wjwwood
Copy link
Member Author

wjwwood commented Jul 24, 2019

@stonier FYI I had to disable emulate_tty by default right now, so that I can get some fixes in and at least have the feature available. I'll keep poking at the issue and try to get it so that we can re-enable it by default.

@wjwwood
Copy link
Member Author

wjwwood commented Jul 24, 2019

CI test failures are from new overnight linter warnings.

@wjwwood wjwwood merged commit 0f81d98 into master Jul 24, 2019
@wjwwood wjwwood deleted the revert-276-revert-265-emulate_tty branch July 24, 2019 21:54
@wjwwood wjwwood mentioned this pull request Jul 25, 2019
Open
@stonier
Copy link
Contributor

stonier commented Jul 26, 2019

@stonier FYI I had to disable emulate_tty by default right now, so that I can get some fixes in and at least have the feature available. I'll keep poking at the issue and try to get it so that we can re-enable it by default.

Understood, thanks.

@wjwwood wjwwood mentioned this pull request Aug 7, 2019
Closed
piraka9011 pushed a commit to aws-ros-dev/launch that referenced this pull request Aug 16, 2019
@wjwwood @piraka9011
Revert "Revert "[execute_process] emulate_tty configurable and defaul…
5b79a9c 
…ts to true"" (ros2#277)

* Revert "Revert "[execute_process] emulate_tty configurable and defaults to true (ros2#265)" (ros2#276)"

This reverts commit 15af530.

* add option to strip ansi escape sequences from output in launch_testing

Signed-off-by: William Woodall <[email protected]>

* move registration of event handlers before including test file

Signed-off-by: William Woodall <[email protected]>

* replace \r\n with \n too, because you get this in emulated tty mode

Signed-off-by: William Woodall <[email protected]>

* fix a new test failure due a change in how pytest represents exceptions

See: pytest-dev/pytest#5579

Signed-off-by: William Woodall <[email protected]>

* refactor to not use YAML in eval of emulate_tty option

Signed-off-by: William Woodall <[email protected]>

* fix typo

Signed-off-by: William Woodall <[email protected]>

* refactor emulate_tty tests and test constructor argument

Signed-off-by: William Woodall <[email protected]>

* change default for emulate_tty to be False and fix warnings

Signed-off-by: William Woodall <[email protected]>
@wjwwood wjwwood mentioned this pull request Aug 24, 2019
Merged
nuclearsandwich added a commit that referenced this pull request Sep 4, 2019
@nuclearsandwich
Fix a test failure due to a change in how pytest represents exceptions.
a04cabe 
This is a cherry-pick of 7b13334 from
#277 to resolve a test failing on CI.

Signed-off-by: Steven! Ragnarök <[email protected]>
nuclearsandwich added a commit that referenced this pull request Sep 4, 2019
@nuclearsandwich
[Dashing] Restore support for Python 3.5 (#324)
7f0d05f 
* Fix Python 3.5 compatibility by removing unsupported type hints.

Some of the type hints introduced in Dashing are not compatible with
Python 3.5. Where possible I tried to convert the hints to type comments
but for the type hints within function argument lists that did not
appear to be possible.

This is going to make backporting future changes more challenging since
we will have to:
1. Review the backported code for incompatible type hints
2. Reconcile merge conflicts caused by the removal of type hints on
the Dashing development branches.

* Convert list comprehension to generator (#300)

Addresses flake8 C412 errors introduced by flake8-comprehension 2.2.0

* Remove type comments longer than 100 chars.

It isn't clear to me from [PEP-0484] how to reconcile overlong lines
with type comments.

[PEP-0484]: https://www.python.org/dev/peps/pep-0484/

* Don't warn on imports used for type comments.

According to [PEP-0484] types used in type comments must be imported or
defined in the current module. Since flake8 is not type-comment aware
(at least in Dashing PR jobs) I've noqa'd the imports.

[PEP-0484]: https://www.python.org/dev/peps/pep-0484/

* Remove invalid trailing comma.

* Fix a test failure due to a change in how pytest represents exceptions.

This is a cherry-pick of 7b13334 from
#277 to resolve a test failing on CI.

Signed-off-by: Steven! Ragnarök <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hidmic hidmic hidmic approved these changes

Assignees

@wjwwood wjwwood

Labels
bug Something isn't working in review Waiting for review (Kanban column)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wjwwood @dirk-thomas @pbaughman @stonier @hidmic