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

Rewrite tutorial with MBS instead of sl4a. #195

Merged
merged 4 commits into from
May 2, 2017
Merged

Rewrite tutorial with MBS instead of sl4a. #195

merged 4 commits into from
May 2, 2017

Conversation

xpconanfan
Copy link
Collaborator

@xpconanfan xpconanfan commented May 2, 2017

Fixes #103


This change is Reviewable

@xpconanfan xpconanfan added the docs label May 2, 2017
@xpconanfan xpconanfan self-assigned this May 2, 2017
@adorokhine
Copy link
Collaborator

Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions.


docs/tutorial.md, line 243 at r1 (raw file):

            self.ads, label='discoverer')
        # Sets the tag that represents this device in logs.
        self.discoverer.debug_tag = 'discoverer'

Should we be encouraging this in the tutorial? It removes the device serial number from the logs so it makes it more difficult to correlate later events to earlier (setup) events, more difficult to correlate test logs to logcats, and more difficult to debug in general.


docs/tutorial.md, line 267 at r1 (raw file):

        discovered_devices = self.discoverer.mbs.btDiscoverAndGetResults()
        self.discoverer.log.debug('Found Bluetooth devices: %s',
                                  discovered_devices)

Should we use pprint to make this prettier? This is going to be pretty hard to read


docs/tutorial.md, line 269 at r1 (raw file):

                                  discovered_devices)
        discovered_names = [device['Name'] for device in discovered_devices]
        logging.info('Verify that the target is discovered by the discoverer.')

Verifying (otherwise sounds like an instruction to the user)


docs/tutorial.md, line 312 at r1 (raw file):

the hardware address, see whether we can pair devices, transfer files, etc.).
 
# Event Dispatcher

No more mention of event dispatcher? Should we replace it with a tutorial about async rpc?


Comments from Reviewable

@xpconanfan
Copy link
Collaborator Author

Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions.


docs/tutorial.md, line 243 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Should we be encouraging this in the tutorial? It removes the device serial number from the logs so it makes it more difficult to correlate later events to earlier (setup) events, more difficult to correlate test logs to logcats, and more difficult to debug in general.

I've been using this in my tests and from my experience, it makes debugging easier. Rather than having to look for the serial (which often are similar among same model devices) in logs, I could look for a word that actually makes sense and not lose track of which serial is supposed to play which role.

There are improvements that can be made to this feature to make certain correlation easier. But I disagree about this making debug more difficult in general.


docs/tutorial.md, line 267 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Should we use pprint to make this prettier? This is going to be pretty hard to read

Done.


docs/tutorial.md, line 312 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

No more mention of event dispatcher? Should we replace it with a tutorial about async rpc?

AsyncRpc has its own tutorial here

That is more of a tutorial for snippet lib, maybe it can be linked instead of pasted here?


Comments from Reviewable

@xpconanfan
Copy link
Collaborator Author

Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions.


docs/tutorial.md, line 269 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Verifying (otherwise sounds like an instruction to the user)

Done.


Comments from Reviewable

@adorokhine
Copy link
Collaborator

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


docs/tutorial.md, line 243 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

I've been using this in my tests and from my experience, it makes debugging easier. Rather than having to look for the serial (which often are similar among same model devices) in logs, I could look for a word that actually makes sense and not lose track of which serial is supposed to play which role.

There are improvements that can be made to this feature to make certain correlation easier. But I disagree about this making debug more difficult in general.

I agree that it makes it easier to read the test log, at the cost of making it hard to correlate setup with test events and harder to correlate test logs to logcats. Can we keep both serial and tag in the logs? That should address all the concerns right?


docs/tutorial.md, line 312 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

AsyncRpc has its own tutorial here

That is more of a tutorial for snippet lib, maybe it can be linked instead of pasted here?

sg, let's link it


Comments from Reviewable

@xpconanfan
Copy link
Collaborator Author

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


docs/tutorial.md, line 243 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

I agree that it makes it easier to read the test log, at the cost of making it hard to correlate setup with test events and harder to correlate test logs to logcats. Can we keep both serial and tag in the logs? That should address all the concerns right?

That seems like a different discussion about a feature change, which is not related to this PR?

what specifically do you mean by "correlate setup with test events" though?


docs/tutorial.md, line 312 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

sg, let's link it

Done.


Comments from Reviewable

@adorokhine
Copy link
Collaborator

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


docs/tutorial.md, line 243 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

That seems like a different discussion about a feature change, which is not related to this PR?

what specifically do you mean by "correlate setup with test events" though?

It's happening in this PR because we're setting the debug_tag in this PR. :)

I mean that everything in start_services, such as starting up the snippet shell, is logged against the serial number. Test events however are logged against the tag, and there's nothing that links the tag with the serial number. So if you're seeing suspicious "callee" logs and want to go see the logcat for that phone, there's no way to know which one that is. Same if you want to see what port it was assigned to, or see the output from starting the snippet process, or anything like that.


Comments from Reviewable

@xpconanfan
Copy link
Collaborator Author

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


docs/tutorial.md, line 243 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

It's happening in this PR because we're setting the debug_tag in this PR. :)

I mean that everything in start_services, such as starting up the snippet shell, is logged against the serial number. Test events however are logged against the tag, and there's nothing that links the tag with the serial number. So if you're seeing suspicious "callee" logs and want to go see the logcat for that phone, there's no way to know which one that is. Same if you want to see what port it was assigned to, or see the output from starting the snippet process, or anything like that.

I want to set the tag in this PR since the example already focus on the role of the devices by using the "label".

The strategy is to propagate the tag into those lower level module loggers. I'd rather not block this tutorial on that discussion.


Comments from Reviewable

@adorokhine
Copy link
Collaborator

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


docs/tutorial.md, line 243 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

I want to set the tag in this PR since the example already focus on the role of the devices by using the "label".

The strategy is to propagate the tag into those lower level module loggers. I'd rather not block this tutorial on that discussion.

If we're going to be recommending people to set tags, we need to fix the debugging issues to make sure we're giving people sound advice.

What do you mean by "propagate the tag into those lower level module loggers"? How does that differ from what we do today?


Comments from Reviewable

@xpconanfan
Copy link
Collaborator Author

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


docs/tutorial.md, line 243 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

If we're going to be recommending people to set tags, we need to fix the debugging issues to make sure we're giving people sound advice.

What do you mean by "propagate the tag into those lower level module loggers"? How does that differ from what we do today?

I do want to recommend this to people bc it is useful today.

We have problem in our adb logcat collection, doesn't mean we don't collect logcat at all. If everything has to be perfect before it can be used, then we'd have nothing to use.


Comments from Reviewable

@adorokhine
Copy link
Collaborator

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


docs/tutorial.md, line 294 at r2 (raw file):
How do you get a shell where this line works? snippet_shell.py would create an object called 'snippet' or 's', not 'device', so if this line is intended to be done from the snippet shell we should probably change it to:

$ snippet_shell.py 'com.google.android.mobly.snippet.bundled'
>>> print(s.help())

Comments from Reviewable

@adorokhine
Copy link
Collaborator

:lgtm:


Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@xpconanfan xpconanfan merged commit 09711f6 into master May 2, 2017
@xpconanfan xpconanfan deleted the tutorial branch May 2, 2017 23:52
@xpconanfan xpconanfan modified the milestone: Mobly Release 1.3.1 May 4, 2017
@xpconanfan xpconanfan modified the milestones: Mobly Release 1.3.1, Mobly Release 1.4 May 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants