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

Use self.is_adb_detectable() to prevent build_info from None #923

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

minghsikang
Copy link
Contributor

@minghsikang minghsikang commented Jun 1, 2024

The logic now is trying to use build_info when it is not in bootloader, but if the device isn't detectable in neither adb nor fastboot, the exception occurs.


This change is Reviewable

@xpconanfan
Copy link
Collaborator

The logic is still trying to access build_info right? What's the difference?

@minghsikang
Copy link
Contributor Author

The logic is still trying to access build_info right? What's the difference?

The difference is that we can only use build_info when the device is detectable. Otherwise, the build_info is None.

Here is the context:
when I tried to init android device, is_rootable could raise exception if the device is not found.

return not self.is_bootloader and self.build_info['debuggable'] == '1'

That's why it is not in bootloader but still trying to access build_info

Hence, I come up with two solutions

  1. change not is_bootloader to is_adb_detectable to make sure we won't access build_info when it is None. (current change)
  2. add a parameter to not use root_adb in init method.

WDYT?

@mhaoli
Copy link
Collaborator

mhaoli commented Jun 3, 2024

when I tried to init android device, is_rootable could raise exception if the device is not found.

Are you meaning you are initializing an AndroidDevice object while there is no corresponding Android device?

We assume each AndroidDevice object corresponds to one Android device. What are you going to do if there's no Android device?

@minghsikang
Copy link
Contributor Author

Are you meaning you are initializing an AndroidDevice object while there is no corresponding Android device?

Partially correct. There is the android device, but it is not detectable by adb or fastboot (even not in rom recovery mode and need ftdi commands to bring it back).

We assume each AndroidDevice object corresponds to one Android device. What are you going to do if there's no Android device?

We will use some other recover scripts/tools to bring it back to fastboot or android and then use Android device.

Copy link
Collaborator

@mhaoli mhaoli left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @minghsikang)


tests/lib/mock_android_device.py line 90 at r1 (raw file):

      installed_packages=None,
      instrumented_packages=None,
      detectable=True,

Renaming this to adb_detectable will be more accurate?

@mhaoli
Copy link
Collaborator

mhaoli commented Jun 3, 2024

Are you meaning you are initializing an AndroidDevice object while there is no corresponding Android device?

Partially correct. There is the android device, but it is not detectable by adb or fastboot (even not in rom recovery mode and need ftdi commands to bring it back).

We assume each AndroidDevice object corresponds to one Android device. What are you going to do if there's no Android device?

We will use some other recover scripts/tools to bring it back to fastboot or android and then use Android device.

I see.

I think in Mobly repo this change makes sense to prevent using a None as a dict.

But for your usage, sounds like it would make more sense to first recover the device and then initialize AndroidDevice object?

@minghsikang
Copy link
Contributor Author

But for your usage, sounds like it would make more sense to first recover the device and then initialize AndroidDevice object?

Totally agree.

…is not in bootloader.

The logic now is trying to use build_info when it is not in bootloader,
but if the device isn't detectable in neither adb nor fastboot, the
exception occurs.
Copy link
Collaborator

@mhaoli mhaoli left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @minghsikang)

Copy link
Contributor Author

@minghsikang minghsikang left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @mhaoli and @minghsikang)


tests/lib/mock_android_device.py line 90 at r1 (raw file):

Previously, mhaoli (Minghao Li) wrote…

Renaming this to adb_detectable will be more accurate?

Done.

@minghsikang
Copy link
Contributor Author

It seems need your help to resolve the comment.

@mhaoli mhaoli merged commit 0911857 into google:master Jun 4, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants