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

fix: ADB.pidof integration issue with grep and CRLF edge case #927

Merged
merged 9 commits into from
Sep 26, 2018

Conversation

noomorph
Copy link
Collaborator

@noomorph noomorph commented Sep 6, 2018

Resolves #924

@noomorph noomorph changed the title [WIP] fix: ADB.pidof integration issue with grep and CRLF edge case (fixes #924 [WIP] fix: ADB.pidof integration issue with grep and CRLF edge case (fixes #924) Sep 6, 2018
@noomorph noomorph changed the title [WIP] fix: ADB.pidof integration issue with grep and CRLF edge case (fixes #924) [WIP] fix: ADB.pidof integration issue with grep and CRLF edge case Sep 6, 2018
@noomorph noomorph changed the title [WIP] fix: ADB.pidof integration issue with grep and CRLF edge case fix: ADB.pidof integration issue with grep and CRLF edge case Sep 7, 2018
@tomatobrown
Copy link

any eta on getting this merged?

@noomorph
Copy link
Collaborator Author

noomorph commented Sep 11, 2018

@tomatobrown, technically, this PR is ready, but two things did not happen:

  1. it was not approved by maintainers
  2. and honestly, I'm still confused about what's happening in your environment with CRLF line endings from Android Emulator on Mac. It is not supposed to happen at all. I think Rotem is hesitant to approve also because of this concern.

await adb.unlockScreen(deviceId);
}

describe('when unlocking an awake and unlocked device', function() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dumpsys power might also emit CRLF output, and this means that we are going into a wrong direction.

@noomorph
Copy link
Collaborator Author

noomorph commented Sep 14, 2018

Important

I have erased the previous implementation in favor of a simpler one, using adb exec-out instead of adb shell. It feels right and if it proves to be working across all major platforms, I'll be happy to merge it soon.

@noomorph noomorph force-pushed the noomorph/fix-crlf-grep-issue branch 2 times, most recently from 9246246 to cbb7d70 Compare September 14, 2018 16:05
@noomorph
Copy link
Collaborator Author

Unfortunately, exec-out had issues of its own, with lower API levels, with handling of ps command (the results were different from adb shell on some of Android versions). So, I'm trying now a third approach - to run grep inside adb shell itself and replace CRLF with LF for every stdout string we get.

Still on it.

@noomorph
Copy link
Collaborator Author

Guys, sorry for the delay. I just have finished the other unrelated but highly important task, so I finally can start sorting out the issues one by one. ETA - tomorrow I make a one more bugfix to PR and merge it

@noomorph
Copy link
Collaborator Author

noomorph commented Sep 20, 2018

Generally speaking, in this pull request there are already multiple various fixes:

  • adb install throws an error if it went wrong
  • pidof() method uses internal Android's grep, eliminating by this CRLF issue at all
  • getFileSize() (used by artifacts) uses du instead of wc for a broader compatibility with Android devices
  • isFileOpen() (used by artifacts) uses lsof | grep which is a more backward-compatible method
  • logcat() (used by log artifact) has a fallback for missing -T <time> parameter in older APIs, and | grep pipe instead of --pid=<pid> parameter
  • exec() helper method replaces CRLF endings if any to fix adb.devices() method on Windows.
  • telnet calls to emulator are logged (sometimes telnet hangs without any message)

TODOs:

  • revert win32Implementation changes in detox/src/utils/pipeCommands.js (leftover from previous PR implementations)
  • fix logical error for API level cache in ADB class - it assumes you always do install which is not true
  • work around lsof issue on some API levels (cannot access /proc/* files)
  • test on API levels 19-28 for Mac, Linux and Windows

I did a lot of testing today manually, unfortunately, CI does not catch dozens of environment integration issues, so I still have to work on this PR tomorrow.

@noomorph noomorph changed the title fix: ADB.pidof integration issue with grep and CRLF edge case [WIP] fix: ADB.pidof integration issue with grep and CRLF edge case Sep 20, 2018
@noomorph noomorph changed the title [WIP] fix: ADB.pidof integration issue with grep and CRLF edge case fix: ADB.pidof integration issue with grep and CRLF edge case Sep 25, 2018
@noomorph noomorph merged commit 3e11f66 into master Sep 26, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Sep 29, 2018
@noomorph noomorph deleted the noomorph/fix-crlf-grep-issue branch April 17, 2019 12:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants