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

ADB log tailing color output #1676

Merged
merged 2 commits into from
Mar 7, 2024
Merged

Conversation

rmartin16
Copy link
Member

@rmartin16 rmartin16 commented Feb 29, 2024

Changes

image

Notes

  • I finally took the time to figure out the regex to account for ANSI sequences in the output.
  • The regex only handles ANSI sequences at the beginning and end of the output....but that's the only place I've seen it.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@rmartin16
Copy link
Member Author

rmartin16 commented Mar 1, 2024

In all its glory in CI: https://github.com/beeware/briefcase/actions/runs/8181483777/job/22371536462?pr=1676#step:28:596

One thing I was considering is that there isn't any way to disable this...adb doesn't seem to respect NO_COLOR, at least. It might be possible to inspect if RIch disabled color...and do the same for adb logcat...

@rmartin16 rmartin16 marked this pull request as ready for review March 1, 2024 00:14
@rmartin16
Copy link
Member Author

rmartin16 commented Mar 7, 2024

I went ahead and added support to detect broader color usage so adb log tailing will use color conditionally.

I wanted the tests to actually confirm the assumptions I'm making about Rich...but Rich is evaluating many aspects of the environment to determine if colors should be enabled. Trying to mock all of them to deal with the permutations of environments in which tests can run start to really question if we're testing those assumptions....or just my ability to mock the hell out of Rich. So, I'll have to dumb down the tests to just verify the outcomes of those assumptions are implemented correctly.

- Update adb log tailing to only use color output if Briefcase is
  already using it
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

It's definitely colourful... :-)

Not much to fault here. I guess we'll learn pretty soon if ANSI sequences appear anywhere in ADB logs other than the start of the line; but we can fix that pretty quickly if we need to.

I'm also not overly concerned about testing how Rich interprets things - at some point, you have to be comfortable that the tools you're using are testing their own behavior, and I think Rich falls in that camp.

@freakboy3742 freakboy3742 merged commit b66ee70 into beeware:main Mar 7, 2024
51 checks passed
@freakboy3742 freakboy3742 mentioned this pull request Mar 7, 2024
4 tasks
@rmartin16 rmartin16 deleted the color-adb branch March 7, 2024 15:37
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.

2 participants