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

Enabled Performance Logs for iOS #3530

Merged
merged 1 commit into from Sep 5, 2014

Conversation

Projects
None yet
4 participants
@axemclion

axemclion commented Aug 30, 2014

Added performance logs for iOS, similar to what Chromedriver does.

Tested using on both mobile web and hybrid (cordova) app

var wd = require('wd');

var b = wd.promiseRemote('http://localhost:4723/wd/hub');
var caps = {
    "platformName": "iOS",
    "platformVersion": "7.1",
    "browserName": "", //"Safari",
    "deviceName": "iPhone Simulator",
    "app": "/sample/platforms/ios/build/emulator/HelloCordova.app.zip",
    "bundleId": "io.cordova.hellocordova",
    autoWebview: true,
    loggingPrefs: {
        performance: 'ALL'
    }
};

b.init(caps).then(function() {
    return b.log('performance');
}).then(function() {
    return require('Q').delay(1000);
}).then(function() {
    return b.log('performance');
}).then(function(data) {
    console.log(data);
}).fin(function() {
    return b.quit();
}).done();
@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Aug 30, 2014

Would love to hear your comments on this pull request. Also added some comments inside the code to explain what is done.

axemclion commented Aug 30, 2014

Would love to hear your comments on this pull request. Also added some comments inside the code to explain what is done.

@axemclion

View changes

Show outdated Hide outdated lib/devices/ios/ios-controller.js
iOSController.setContext = function (name, cb, skipReadyCheck) {
iOSController.setContext = function (name, callback, skipReadyCheck) {
var self = this;
var cb = function(err, res){

This comment has been minimized.

@axemclion

axemclion Aug 30, 2014

Hijacked the callback. Need to start the performance log everytime the context is set or reset. This is when the timeline can be started

@axemclion

axemclion Aug 30, 2014

Hijacked the callback. Need to start the performance log everytime the context is set or reset. This is when the timeline can be started

// Date-Utils: Polyfills for the Date object
require('date-utils');
var MAX_EVENTS = 5000;

This comment has been minimized.

@axemclion

axemclion Aug 30, 2014

Should this be configurable from the command line ? Should we even have a max limit ?

@axemclion

axemclion Aug 30, 2014

Should this be configurable from the command line ? Should we even have a max limit ?

This comment has been minimized.

@jlipps

jlipps Sep 3, 2014

Member

i can't think of any reason to have a limit, but if we do set one, we should make it configurable via another key on the loggingPrefs cap.

@jlipps

jlipps Sep 3, 2014

Member

i can't think of any reason to have a limit, but if we do set one, we should make it configurable via another key on the loggingPrefs cap.

This comment has been minimized.

@axemclion

axemclion Sep 3, 2014

The only reason I would have limits is if the user starts a session with perflogs, and runs the selenium tests for hours. In that case, the events array keeps getting bigger and bigger, eventually running out of space .

@axemclion

axemclion Sep 3, 2014

The only reason I would have limits is if the user starts a session with perflogs, and runs the selenium tests for hours. In that case, the events array keeps getting bigger and bigger, eventually running out of space .

This comment has been minimized.

@jlipps

jlipps Sep 3, 2014

Member

I think the logs should automatically reset every session, right? If someone has one session going for hours, sounds like they need to make sure they've got some serious ram :-)

@jlipps

jlipps Sep 3, 2014

Member

I think the logs should automatically reset every session, right? If someone has one session going for hours, sounds like they need to make sure they've got some serious ram :-)

This comment has been minimized.

@axemclion

axemclion Sep 3, 2014

They do reset everytime the webview is switched, so technically that would be ok. I just wanted to have a safeguard - for example, if used on sauce machines, you would not want the VMs to go out of memory on appium.

@axemclion

axemclion Sep 3, 2014

They do reset everytime the webview is switched, so technically that would be ok. I just wanted to have a safeguard - for example, if used on sauce machines, you would not want the VMs to go out of memory on appium.

This comment has been minimized.

@jlipps

jlipps Sep 3, 2014

Member

You know in this case, why don't we remove the safeguard until we have evidence we need it. We can always add in a server param to set this later, but it seems a bit like overengineering to me at the moment. What do you think?

@jlipps

jlipps Sep 3, 2014

Member

You know in this case, why don't we remove the safeguard until we have evidence we need it. We can always add in a server param to set this later, but it seems a bit like overengineering to me at the moment. What do you think?

@Jonahss

This comment has been minimized.

Show comment
Hide comment
@Jonahss

Jonahss Sep 2, 2014

Contributor

@jlipps @bootstraponline thoughts?
@axemclion Can you correct merge conflicts?

Contributor

Jonahss commented Sep 2, 2014

@jlipps @bootstraponline thoughts?
@axemclion Can you correct merge conflicts?

@bootstraponline

This comment has been minimized.

Show comment
Hide comment
@bootstraponline

bootstraponline Sep 2, 2014

Member

I don't know much about iOS performance logging so @jlipps is probably the preferred reviewer. Is there documentation somewhere that explains debug timelines for iOS? It's not clear to me if this is only for web apps or if it'd work on native.

Member

bootstraponline commented Sep 2, 2014

I don't know much about iOS performance logging so @jlipps is probably the preferred reviewer. Is there documentation somewhere that explains debug timelines for iOS? It's not clear to me if this is only for web apps or if it'd work on native.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Sep 2, 2014

@bootstraponline This is only for Webkit based systems - Safari and WebView based apps. Thats the reason why it starts when the context is set.

axemclion commented Sep 2, 2014

@bootstraponline This is only for Webkit based systems - Safari and WebView based apps. Thats the reason why it starts when the context is set.

@bootstraponline

This comment has been minimized.

Show comment
Hide comment
@bootstraponline

bootstraponline Sep 2, 2014

Member

@axemclion Ok, so are the timelines referring to this? I think this is a cool feature.

Member

bootstraponline commented Sep 2, 2014

@axemclion Ok, so are the timelines referring to this? I think this is a cool feature.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Sep 2, 2014

@bootstraponline - This is correct. I basically did this so that perf numbers can be recorded for iOS also using something like http://github.com/axemclion/browser-perf. It already does that for Android.

axemclion commented Sep 2, 2014

@bootstraponline - This is correct. I basically did this so that perf numbers can be recorded for iOS also using something like http://github.com/axemclion/browser-perf. It already does that for Android.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Sep 2, 2014

@Jonahss - Rebased on the latest master.

axemclion commented Sep 2, 2014

@Jonahss - Rebased on the latest master.

@@ -83,6 +84,7 @@ var iosCaps = [
, 'localizableStringsDir'
, 'interKeyDelay'
, 'showIOSLog'
, 'loggingPrefs'

This comment has been minimized.

@axemclion

axemclion Sep 2, 2014

I see that Android supports enablePerformanceLogging, but the selenium standard (and Chromedriver) seem to call this loggingPrefs. This way, existing chromedriver code need not change.

@axemclion

axemclion Sep 2, 2014

I see that Android supports enablePerformanceLogging, but the selenium standard (and Chromedriver) seem to call this loggingPrefs. This way, existing chromedriver code need not change.

@jlipps

View changes

Show outdated Hide outdated lib/devices/ios/ios.js
@@ -162,6 +162,8 @@ IOS.prototype.setIOSArgs = function () {
null;
this.curOrientation = this.args.initialOrientation;
this.sock = path.resolve(this.args.tmpDir || '/tmp', 'instruments_sock');
this.perfLogEnabled = !!(typeof this.args.loggingPrefs && this.args.loggingPrefs.performance);

This comment has been minimized.

@jlipps

jlipps Sep 3, 2014

Member

typeof will return a string no matter what it is, which will be truthy. think you mean typeof this.args.loggingPrefs === 'object' or somethin

@jlipps

jlipps Sep 3, 2014

Member

typeof will return a string no matter what it is, which will be truthy. think you mean typeof this.args.loggingPrefs === 'object' or somethin

@jlipps

View changes

Show outdated Hide outdated lib/devices/ios/ios-perf-log.js
IosPerfLog.prototype.stopCapture = function() {
this.timelineEvents = null;
return this.remote.startTimeline(cb);

This comment has been minimized.

@jlipps

jlipps Sep 3, 2014

Member

i think you mean for this to be stopTimeline

@jlipps

jlipps Sep 3, 2014

Member

i think you mean for this to be stopTimeline

This comment has been minimized.

@axemclion

axemclion Sep 3, 2014

aaarg ... yes, that is stopTimeline. Will fix this. How did I mess this up !! :rage4:

@axemclion

axemclion Sep 3, 2014

aaarg ... yes, that is stopTimeline. Will fix this. How did I mess this up !! :rage4:

@jlipps

This comment has been minimized.

Show comment
Hide comment
@jlipps

jlipps Sep 3, 2014

Member

I made two logic comments. Everything else looks great, except you'll need to do a bunch of style revision (see CONTRIBUTING.md for example). Travis build failed because grunt lint failed. Please clean up the spacing/style issues as well, then I'll give a final pass and merge. Looks awesome! Thanks for doing this.

Member

jlipps commented Sep 3, 2014

I made two logic comments. Everything else looks great, except you'll need to do a bunch of style revision (see CONTRIBUTING.md for example). Travis build failed because grunt lint failed. Please clean up the spacing/style issues as well, then I'll give a final pass and merge. Looks awesome! Thanks for doing this.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Sep 3, 2014

@jlipps Thanks for taking the time to look at it and the comments. I have fixed them. Now waiting for grunt to finish it.

axemclion commented Sep 3, 2014

@jlipps Thanks for taking the time to look at it and the comments. I have fixed them. Now waiting for grunt to finish it.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Sep 3, 2014

@jlipps Fixed styling issues, and addressed the other comments.

axemclion commented Sep 3, 2014

@jlipps Fixed styling issues, and addressed the other comments.

@jlipps

View changes

Show outdated Hide outdated lib/devices/ios/ios-perf-log.js
var MAX_EVENTS = 5000;
var IosPerfLog = function (remote) {
this.remote = remote;

This comment has been minimized.

@jlipps

jlipps Sep 3, 2014

Member

In this file indentation should be at 2 spaces, not 8. Not sure how lint didn't catch that. Maybe because it's a new file?

@jlipps

jlipps Sep 3, 2014

Member

In this file indentation should be at 2 spaces, not 8. Not sure how lint didn't catch that. Maybe because it's a new file?

This comment has been minimized.

@axemclion

axemclion Sep 3, 2014

Some other errors were actually caught on this file. For example - https://travis-ci.org/appium/appium/jobs/34247319#L587

I will change it to 2 spaces and rebase.

@axemclion

axemclion Sep 3, 2014

Some other errors were actually caught on this file. For example - https://travis-ci.org/appium/appium/jobs/34247319#L587

I will change it to 2 spaces and rebase.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Sep 3, 2014

@jlipps Fixed it to 2 spaces. Waiting for travis to complete and will see if there is anything else for me to fix.

axemclion commented Sep 3, 2014

@jlipps Fixed it to 2 spaces. Waiting for travis to complete and will see if there is anything else for me to fix.

@jlipps

This comment has been minimized.

Show comment
Hide comment
@jlipps

jlipps Sep 3, 2014

Member

OK, sounds good. FWIW, you can run grunt travis on your own and get feedback before pushing too :-)

Member

jlipps commented Sep 3, 2014

OK, sounds good. FWIW, you can run grunt travis on your own and get feedback before pushing too :-)

@jlipps

View changes

Show outdated Hide outdated lib/devices/ios/remote-debugger.js
@@ -450,6 +469,8 @@ RemoteDebugger.prototype.setHandlers = function () {
}
} else if (dataKey.method === "Page.loadEventFired") {
this.pageLoad();
} else if (dataKey.method === "Timeline.eventRecorded"){

This comment has been minimized.

@jlipps

jlipps Sep 3, 2014

Member

need a space before { here

@jlipps

jlipps Sep 3, 2014

Member

need a space before { here

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion commented Sep 4, 2014

@jlipps Done.

@jlipps

This comment has been minimized.

Show comment
Hide comment
@jlipps

jlipps Sep 4, 2014

Member

ok @axemclion, looks great. Finally, we just resolved as an appium dev team that we're not going to merge features without tests / docs. I'd love to see at least one functional test for this in our mobile safari testsuite. it should be pretty straightforward to add; would you mind giving that a shot?

Member

jlipps commented Sep 4, 2014

ok @axemclion, looks great. Finally, we just resolved as an appium dev team that we're not going to merge features without tests / docs. I'd love to see at least one functional test for this in our mobile safari testsuite. it should be pretty straightforward to add; would you mind giving that a shot?

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Sep 4, 2014

@jlipps Makes sense. Is there an example test that you can point me to, where I can get started?

axemclion commented Sep 4, 2014

@jlipps Makes sense. Is there an example test that you can point me to, where I can get started?

@jlipps

This comment has been minimized.

Show comment
Hide comment
@jlipps

jlipps Sep 4, 2014

Member

yeah, i would take a look at special-caps-specs.js in test/functional/ios/safari/webview.js. you should be able to just add another it block (and add the loggingPrefs cap in the main describe so they're active for your case.

Member

jlipps commented Sep 4, 2014

yeah, i would take a look at special-caps-specs.js in test/functional/ios/safari/webview.js. you should be able to just add another it block (and add the loggingPrefs cap in the main describe so they're active for your case.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Sep 5, 2014

@jlipps Added test. Ran it using DEVICE=ios71 mocha -t 60000 -R spec test/functional/ios/safari/webview/special-caps-specs.js

axemclion commented Sep 5, 2014

@jlipps Added test. Ran it using DEVICE=ios71 mocha -t 60000 -R spec test/functional/ios/safari/webview/special-caps-specs.js

@@ -9,24 +9,46 @@ var env = require('../../../../helpers/env.js'),
ChaiAsserter = require('../../../../helpers/asserter.js').ChaiAsserter;
describe('safari - webview - special capabilities', function () {
var driver;

This comment has been minimized.

@axemclion

axemclion Sep 5, 2014

Moved this test to a new describe block since I think that ignorewarning capability is tested independent of performance test.

@axemclion

axemclion Sep 5, 2014

Moved this test to a new describe block since I think that ignorewarning capability is tested independent of performance test.

@axemclion

View changes

Show outdated Hide outdated test/functional/ios/safari/webview/special-caps-specs.js
.title()
.should.eventually.contain("I am another page title");
describe('performance logs', function() {
var driver;

This comment has been minimized.

@axemclion

axemclion Sep 5, 2014

Included a new performance test.

@axemclion

axemclion Sep 5, 2014

Included a new performance test.

loadWebView(specialCaps, driver).nodeify(done);
});
it('should not display a phishing warning with safariIgnoreFraudWarning @skip-chrome', function (done) {

This comment has been minimized.

@axemclion

axemclion Sep 5, 2014

This test has not really changed, just moved to a separaet describe block to isolate it.

@axemclion

axemclion Sep 5, 2014

This test has not really changed, just moved to a separaet describe block to isolate it.

jlipps added a commit that referenced this pull request Sep 5, 2014

Merge pull request #3530 from axemclion/perf
Enabled Performance Logs for iOS

@jlipps jlipps merged commit 3a67eef into appium:master Sep 5, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@jlipps

This comment has been minimized.

Show comment
Hide comment
@jlipps

jlipps Sep 5, 2014

Member

awesome, thanks!

Member

jlipps commented Sep 5, 2014

awesome, thanks!

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Sep 5, 2014

Thanks for merging this. I can now use http://github.com/axemclion/browser-perf to record perf stats for iOS hybrid and safari sites too. 👍

axemclion commented Sep 5, 2014

Thanks for merging this. I can now use http://github.com/axemclion/browser-perf to record perf stats for iOS hybrid and safari sites too. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment