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

Adding ability to run adb reverse in mobly, fixes #900 #901

Closed
wants to merge 0 commits into from

Conversation

ssachinsunder
Copy link
Contributor

@ssachinsunder ssachinsunder commented Oct 23, 2023

Adding AdbProxy.reverse() method to run adb reverse via Mobly framework.


This change is Reviewable

@ssachinsunder ssachinsunder marked this pull request as draft October 23, 2023 20:25
@ssachinsunder ssachinsunder marked this pull request as ready for review October 23, 2023 20:27
@ssachinsunder ssachinsunder reopened this Oct 23, 2023
@xpconanfan xpconanfan added this to the Mobly Release 1.12.3 milestone Oct 23, 2023
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 2 files reviewed, 1 unresolved discussion (waiting on @ssachinsunder and @xpconanfan)


tests/mobly/controllers/android_device_lib/adb_test.py line 634 at r1 (raw file):

  def test_reverse(self):
    with mock.patch.object(adb.AdbProxy, '_exec_cmd') as mock_exec_cmd:
      adb.AdbProxy().reverse(MOCK_SHELL_COMMAND)

Add an assertion to check that correct arguments are passed to _exec_cmd?

Code quote:

adb.AdbProxy().reverse(MOCK_SHELL_COMMAND)

@ssachinsunder
Copy link
Contributor Author

Updated the test_reverse() andtest_forward() with assert statements.

Copy link
Contributor Author

@ssachinsunder ssachinsunder 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 2 files reviewed, 2 unresolved discussions (waiting on @mhaoli, @ssachinsunder, and @xpconanfan)


tests/mobly/controllers/android_device_lib/adb_test.py line 634 at r1 (raw file):

Previously, mhaoli (Minghao Li) wrote…

Add an assertion to check that correct arguments are passed to _exec_cmd?

Done.

mhaoli
mhaoli previously approved these changes Oct 25, 2023
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.

Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: 1 of 2 files reviewed, 4 unresolved discussions (waiting on @ssachinsunder and @xpconanfan)


tests/mobly/controllers/android_device_lib/adb_test.py line 630 at r3 (raw file):

  def test_forward(self):
    with mock.patch.object(adb.AdbProxy, '_exec_cmd') as mock_exec_cmd:
      adb.AdbProxy().forward(MOCK_SHELL_COMMAND)

Thanks for updating the test case test_forward.

How about using args that are similiar to real forward command and reverse command in the test cases?

e.g.
adb.AdbProxy().forward(['tcp:12345', 'tcp:23456'])
adb.AdbProxy().reverse(['tcp:12345', 'tcp:23456'])

Code quote:

MOCK_SHELL_COMMAND

tests/mobly/controllers/android_device_lib/adb_test.py line 638 at r3 (raw file):

  def test_reverse(self):
    with mock.patch.object(adb.AdbProxy, '_exec_cmd') as mock_exec_cmd:
      adb.AdbProxy().reverse(MOCK_SHELL_COMMAND)

ditto here

Copy link
Contributor Author

@ssachinsunder ssachinsunder 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: 1 of 2 files reviewed, 4 unresolved discussions (waiting on @mhaoli and @xpconanfan)


tests/mobly/controllers/android_device_lib/adb_test.py line 630 at r3 (raw file):

Previously, mhaoli (Minghao Li) wrote…

Thanks for updating the test case test_forward.

How about using args that are similiar to real forward command and reverse command in the test cases?

e.g.
adb.AdbProxy().forward(['tcp:12345', 'tcp:23456'])
adb.AdbProxy().reverse(['tcp:12345', 'tcp:23456'])

Done.


tests/mobly/controllers/android_device_lib/adb_test.py line 638 at r3 (raw file):

Previously, mhaoli (Minghao Li) wrote…

ditto here

Done.

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.

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @xpconanfan)

mhaoli
mhaoli previously approved these changes Oct 26, 2023
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: all files reviewed, 1 unresolved discussion (waiting on @xpconanfan)

@xpconanfan
Copy link
Collaborator

Please rebase and resolve the conflict so we can merge

Copy link
Contributor Author

@ssachinsunder ssachinsunder left a comment

Choose a reason for hiding this comment

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

It seems like there was a switch in py formatting tool. How do I use the new tool (pyink) to ensure formatting is right?

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

@ssachinsunder
Copy link
Contributor Author

Sorry closed this accidentally by discarding commits via GitHub UI (didn't expect that to happen). Created a new pull request - #903

@xpconanfan xpconanfan removed this from the Mobly Release 1.12.3 milestone Dec 12, 2023
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.

3 participants