Further improve time until first test gets started by not blocking on get_appinfo()

RESOLVED WONTFIX

Status

Testing Graveyard
Mozmill
RESOLVED WONTFIX
6 years ago
2 years ago

People

(Reporter: whimboo, Assigned: jcmojicar, Mentored)

Tracking

({perf})

Details

(Whiteboard: [lang=py,js][good first bug], URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
On bug 753752 we did a good start in reducing the startup time for the first test, but we are still waiting about 2s until the application information has been transferred. That means the python process is blocked on it before the first test can be started.

We should remove this blocking state by letting mozmill transfer the application_information asynchronously via a new event.

It's not blocking us right now but would be great to have for Mozmill 2.1.

Comment 1

5 years ago
Hi, can I try to fix this bug?
(Reporter)

Comment 2

5 years ago
Hi Usurelu. We would appreciate if you could work on this bug. Please let me know if it would be still on your plan. If you need more information please ask or join us on IRC in the #automation or #ateam channel. I will be away the next two weeks but others might also help out as needed. CC'ing some folks.

Comment 3

5 years ago
how can i help on this? I am new here and i need directives. Let me know if i can do something.
(Reporter)

Comment 4

5 years ago
Since we haven't heard back from Usurelu, you can take up this bug if you want. Please check my comment 2 how to get in contact with us. Until then check the URL and get familiar with Mozmill itself. It will help in getting this thing fixed.

https://github.com/mozilla/mozmill
https://developer.mozilla.org/en/Mozmill
https://wiki.mozilla.org/Auto-tools/Projects/Mozmill/RepoSetup

You can ask at any time if something is unclear to you.
(Assignee)

Comment 5

4 years ago
Hi, i would like to work on this. I have already followed the steps described above (forking the mozmill repo, installing git, cloning to a local repo, creating a virtualenv and running the setup_development.py to install some python modules).

At this point i need some directions to carry on.

Hope i can help you guys with this.
Flags: needinfo?
(Reporter)

Comment 6

4 years ago
Hi jcmojicar. It's good to see that you have interests in working on this bug. Let me assign it to you now.

Regarding assistance I will be always around for answering your questions. Best would be if you join us on IRC in the #automation channel. It would make it easier to discuss details for this issue. But as a starter please have a look at the existent event handlers (listeners) in the Python code and how those events are getting triggered in the Mozmill extension.
Assignee: nobody → jcmojicar
Status: NEW → ASSIGNED
Flags: needinfo?
(Reporter)

Updated

4 years ago
Keywords: perf
(Assignee)

Comment 7

4 years ago
hi Henrik, this what i tried yesterday:

In the mozmill __init__.py:

1- changed the get_appinfo() method by removing the return sentence and the app_info.update... 

2- Change the new listener created for receiving the message to include the app_info.update...

In mozmill.js:

1- in getApplicationDetails(), removed the return sentence and added a fireEvent as frame.events.fireEvent(... -> (based on another fireEvent found in mozmill.js).

In frame.js:

1- Register the listener by adding an entry in broker.addObject (in the same format as the existing ones).
2- Created a function events.<event name> that calls the fireEvent function (as done in the other listeners in frame.js.

This is not working the event is never received back in Python and, dont know how to add some debugging to the js to know whats happening in there.
(Assignee)

Comment 8

4 years ago
In mozmill __init__.py, mozmill.js is imported first than frame.js ; in mozmill.js i added:

var frame = {};     Cu.import('resource://mozmill/modules/frame.js', frame);

after that the getAppInformation Event is received in python and the app details are stored in app_info.

I dont see a big improvement over the original code (sometimes 1 second faster to start the first test, and most of the times its the same time than the original code), but now the calling of get_appinfo is not blocking the rest of the code.

If in the original code i move the get_appinfo call after the tests are run, the start of the first test is between 1 or 2 seconds faster, so i dont know if something else is blocking, i was trying to insert a delay in the mozmill.js -> getApplicationDetails function to check if the asynch event can be received in python later (to check that is really not blocking) but could not find how to do it.
(Assignee)

Comment 9

4 years ago
Created attachment 8350370 [details]
mozmill __init__.py, with a non-blocking call to get_appinfo()
Attachment #8350370 - Flags: review?(hskupin)
(Assignee)

Comment 10

4 years ago
Created attachment 8350371 [details]
mozmill.js, with the generation of an event in the getApplicationDetails() function.

I added a timeout with nsITimer, and checked that the app info is received asynch in Python.
Attachment #8350371 - Flags: review?(hskupin)
(Assignee)

Comment 11

4 years ago
Created attachment 8350372 [details]
frame.js with the modifications for the getAppInformation event generation
Attachment #8350372 - Flags: review?(hskupin)
(Reporter)

Comment 12

4 years ago
(In reply to jcmojicar from comment #7)
> hi Henrik, this what i tried yesterday:

All that sounds reasonable. But I would like to have a look at a complete patch so I could also test it. Therefore please try to work with git to create the patch for all the modifications you did. This is the required method compared to attaching all modified files. I will remove my review flags for now.

> This is not working the event is never received back in Python and, dont
> know how to add some debugging to the js to know whats happening in there.

If you want to debug the JS side you can add `dump('foobar')` calls, which will be printed to stdout.
(Reporter)

Updated

4 years ago
Attachment #8350372 - Flags: review?(hskupin)
(Reporter)

Updated

4 years ago
Attachment #8350371 - Flags: review?(hskupin)
(Reporter)

Updated

4 years ago
Attachment #8350370 - Flags: review?(hskupin)
(Reporter)

Comment 13

4 years ago
Oh, and please have a look at the following link in how to create the patch. If you have questions regarding git please ask or find me on IRC in the #automation channel.
(Assignee)

Comment 14

4 years ago
Created attachment 8355930 [details] [diff] [review]
0001-Modified-__init__.py-frame.js-and-mozmill.js-to-have.patch

Patch for a non-blocking call to get_appinfo().
Attachment #8350370 - Attachment is obsolete: true
Attachment #8350371 - Attachment is obsolete: true
Attachment #8350372 - Attachment is obsolete: true
Attachment #8355930 - Flags: review?(hskupin)
(Assignee)

Comment 15

4 years ago
Just added the patch, i followed the instructions on https://wiki.mozilla.org/Auto-tools/Projects/Mozmill/RepoSetup.

Sorry for the previous uploads, did not know about the patch generation yet. If you find anything that should not be there please let me know.
(Reporter)

Comment 16

4 years ago
This bug will not be taken for version 2.1. We might want to re-evaluate for 2.2.
Whiteboard: [mozmill-2.1?][mentor=whimboo][lang=py,js][good first bug] → [mozmill-2.2?][mentor=whimboo][lang=py,js][good first bug]
(Assignee)

Comment 17

4 years ago
hi henrik, is there anything else i can do for this bug?.
(Reporter)

Comment 18

4 years ago
Sorry that i wasn't able to review your patch yet. But we have massive problems with Mozmill and restart tests and have to get them fixed first. This is mostly taking up my time. I will try to get to a review ASAP.
Mentor: hskupin@gmail.com
Whiteboard: [mozmill-2.2?][mentor=whimboo][lang=py,js][good first bug] → [mozmill-2.2?][lang=py,js][good first bug]
Whimboo, can you review this patch?
Flags: needinfo?(hskupin)
(Reporter)

Comment 20

4 years ago
This slipped through. Sorry. I will get to it in a couple of days, when we start the main work for Mozmill 2.1. I really would like to see this included.
Flags: needinfo?(hskupin)
Henrik,  this is a really old review- also a mozmill specific thing; quite possibly this doesn't have a lot of value given the changes required for e10s.
(Reporter)

Comment 22

3 years ago
Comment on attachment 8355930 [details] [diff] [review]
0001-Modified-__init__.py-frame.js-and-mozmill.js-to-have.patch

Review of attachment 8355930 [details] [diff] [review]:
-----------------------------------------------------------------

I feel bad that this review got delayed that massively. I even had a tab still open for that review but actually never submitted my comments. :( 

Given the last comment from Joel and that we are really working hard in getting Marionette into a state that we can convert our Mozmill tests, I would propose that we may not want to fix this bug for Mozmill. As you can see in my inline comments it can cause some weird behavior.

jcmojicar, if you are still interested in another bug please let us know or look at the open automation bugs on bugsahoy: http://www.joshmatthews.net/bugsahoy/?automation=1&unowned=1&simple=1

::: mozmill/mozmill/__init__.py
@@ +450,4 @@
>  
>          try:
>              mozmill = jsbridge.JSObject(self.bridge, js_module_mozmill)
> +            mozmill.getApplicationDetails()

Hm, now that we return the data asynchronously we need a way to ensure that we really got this information returned. If a test would finish kinda quickly and Firefox closes, we might not have any information at all. Maybe we can have a kind of wait for method in stop runner?

https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/__init__.py#L609

::: mozmill/mozmill/extension/resource/driver/mozmill.js
@@ +30,4 @@
>  var os = {};          Cu.import('resource://mozmill/stdlib/os.js', os);
>  var utils = {};       Cu.import('resource://mozmill/stdlib/utils.js', utils);
>  var windows = {};     Cu.import('resource://mozmill/modules/windows.js', windows);
> +var frame = {};     Cu.import('resource://mozmill/modules/frame.js', frame);

nit: please bring this in alphabetical order

::: mozmill/mozmill/extension/resource/modules/frame.js
@@ +312,5 @@
>    events.fireEvent('skip', reason);
>  }
>  
> +events.getAppInformation = function (appDetails) {
> +  events.fireEvent('getAppInformation', appDetails);

The event type should be 'appInfo' or such, so without the actual action.
Attachment #8355930 - Flags: review?(hskupin) → review-
(Reporter)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
Whiteboard: [mozmill-2.2?][lang=py,js][good first bug] → [lang=py,js][good first bug]
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.