634582 - Add support for running endurance tests

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Crowd Extension, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

We need the ability to run the endurance tests from the crowd extension.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Aaron, could you help me and add this basic feature to the Mozmill Crowd extension? We would also need two more entries in the preferences dialog which can be used from users to specify the delay and iterations.

For now it can be statically included but in the future I would like to make use of the config file which has to specify supported test-runs.

This fix is required for the testday next week.

Comment 2

[Aaron Train [:aaronmt]]

Without looking too much over what this would involve, I take it the following would need to be done. Let's list:

https://github.com/whimboo/mozmill-crowd/blob/master/extension/chrome/content/mozmill-crowd.js#L48 - Add the script here

https://github.com/whimboo/mozmill-crowd/blob/master/extension/chrome/content/mozmill-crowd.js#L259 - Make a conditional for the endurance test run, specifying which default values for iterations and duration? 

https://github.com/whimboo/mozmill-crowd/blob/master/extension/defaults/preferences/mozmill-crowd.js#L7 - Adding Dave's reporting server address

https://github.com/whimboo/mozmill-crowd/blob/master/extension/chrome/content/preferences.xul - Adding the duration/iterations preferences in xul

Is there any other backend changes that would be needed in the extension to support endurance runs?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Except the reporting address, everything sounds fine. We hope to have the endurance views available on our public dashboard by next week, so users won't have to send reports to Dave's dashboard. Otherwise they will only have to replace the URL in the preferences dialog.

Beside the mentioned items, there are no other changes necessary.

Comment 4

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - Endurance test support in mozmill-crowd

This patch adds support into the extension in running endurance tests as a selectable option in the UI. 

I have added preferences that set the iteration count (default: 1) and delay (default: 0) as selectable in the add-on's preferences. The preferences have a reasonable max limit on iterations up to 9999 (4 digits), and a reasonable max limit on delay (6 digits), which would be like 999999 milliseconds ~ 16.6 minutes.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 520067 [details] [diff] [review]
Patch v1 - Endurance test support in mozmill-crowd

>-  name : "Add-ons Test-run", script: "testrun_addons.py" }
>+  name : "Add-ons Test-run", script: "testrun_addons.py" }, {
>+  name : "Endurance Test-run", script: "testrun_endurance.py" }

Please put the Endurance tests before the add-ons one. Those have a higher priority.

>+       <label id="iterationsMessage"

Does the label need an id?

>+       <textbox id="iterationsAmount"

What about simply iterations as id?

>+                maxlength="4"
>+                min="1"
>+                size="4"
>+                type="number"
>+                preference="extensions.mozmill-crowd.endurance.iterations"/>

I think we should increase it to maxlength=5.

>+       <label id="delayMessage"
>+              control="delayAmount"
>+              value="&enduranceTestrunDelay.label;"/>
>+       <textbox id="delayAmount"

Same as above, and "delay" instead of "delayAmount".

>+                maxlength="6"
>+                min="0"
>+                size="6"
>+                type="number"
>+                preference="extensions.mozmill-crowd.endurance.delay"/>

Keep in mind those are seconds not milliseconds. But better more as too less.

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to comment #5)

> >+                maxlength="6"
> >+                min="0"
> >+                size="6"
> >+                type="number"
> >+                preference="extensions.mozmill-crowd.endurance.delay"/>
> 
> Keep in mind those are seconds not milliseconds. But better more as too less.

The delay parameter is in milliseconds.

Comment 7

[Aaron Train [:aaronmt]]

(In reply to comment #5)
> >+       <label id="iterationsMessage"
> 
> Does the label need an id?
I just placed one in since every other element has one. If it's unnecessary, I can remove it. 

The script expects milliseconds as Dave mentions.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I think we should have a default delay (perhaps 100 milliseconds), as we have seen an issue with memory not being released when there is no delay. Alternatively, we cold go ahead with no delay and tweak later if necessary.

Comment 9

[Aaron Train [:aaronmt]]

(In reply to comment #8)
> I think we should have a default delay (perhaps 100 milliseconds), as we have
> seen an issue with memory not being released when there is no delay.
> Alternatively, we cold go ahead with no delay and tweak later if necessary.

I'd suggest a new bug as this bug is to merely implement based upon what's currently available.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #7)
> (In reply to comment #5)
> > >+       <label id="iterationsMessage"
> > 
> I just placed one in since every other element has one. If it's unnecessary, I
> can remove it. 

Yeah, only add a value and a control attribute to it. That should be fine. 

> The script expects milliseconds as Dave mentions.

Darn. We should change that. The global timeout is given in seconds, so the delay should follow this convention. The Endurance Python script can simply multiply the value with 1000 when storing it in the persisted object.

I agree with at least a delay of 100ms

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I'm confused. If we set delay in seconds then we can't have a default of 100ms, unless the default is 0.1 seconds. Is this what you're suggesting?

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

That's correct.

Comment 13

[Aaron Train [:aaronmt]]

So with 0.1 seconds as the new default, this makes holding the preference (integer) a little annoying. With 0 as the default preference integer, I would have to force change it to 0.1 (100ms), which I don't really like:

var delay = Utils.getPref("extensions.mozmill-crowd.endurance.delay", 0);
if(!delay) delay = 0.1

So, do we want something like that ^, or for the extension, do we want to keep the default at 1s? I would use the label, "Delay (in seconds) to wait before each iteration (1s default)".

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I don't like the idea of a default 1 second delay, it feels like too much.

Comment 15

[Aaron Train [:aaronmt]]

Attachment: Patch v2 - Endurance test support in mozmill-crowd

Even simpler, scratch comment #13. I can store the delay as a string, merely needed for the '0.1' delay default , and the optparser with the specified float type does all the magic conversion

http://docs.python.org/library/optparse.html#optparse-standard-option-types

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Does it mean when you set type='number' for a textbox it will write the value as string into the specified preference?

Comment 17

[Aaron Train [:aaronmt]]

Right. Setting the type on the text-box to 'number' just prohibits anything but numbers from being entered: https://developer.mozilla.org/en/XUL/textbox#a-textbox.type

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 520726 [details] [diff] [review]
Patch v2 - Endurance test support in mozmill-crowd

>+       <label control="iterationsAmount"
>+              value="&enduranceTestrunIterations.label;"/>
>+       <textbox id="iterations"

Please tweak the position both textboxes so they will appear below the the label.

>+       <textbox id="delay"
>+                accesskey="&enduranceTestrunDelay.accesskey;"
>+                maxlength="4"
>+                min="0"
>+                size="4"
>+                type="number"
>+                preference="extensions.mozmill-crowd.endurance.delay"/>

That doesn't work because we lose the fraction part of the value. So the default will end up in '0'. Is there any way to get this accomplished with type=number? If not we should go with milliseconds and convert it to seconds when calling the automation script.

>+<!ENTITY enduranceTestrunDelay.label         "Delay (in seconds) to wait before each iteration (0s default)">

I think it's enough to call out the unit once for the default value. Latter one should be listed as i.e. '(default: 100ms)'. Also rename 'Delay' into 'Time'.

>+<!ENTITY enduranceTestrunIterations.label    "Number of times to repeat each test snippet (1 default)">

What about: 'Iteration count for each test (default: 1)' ?

Comment 19

[Aaron Train [:aaronmt]]

(In reply to comment #18)
> >+       <textbox id="delay"
> >+                accesskey="&enduranceTestrunDelay.accesskey;"
> >+                maxlength="4"
> >+                min="0"
> >+                size="4"
> >+                type="number"
> >+                preference="extensions.mozmill-crowd.endurance.delay"/>
> 
> That doesn't work because we lose the fraction part of the value. So the
> default will end up in '0'. Is there any way to get this accomplished with
> type=number? If not we should go with milliseconds and convert it to seconds
> when calling the automation script.

I can do it with, https://developer.mozilla.org/en/XUL/Attribute/decimalplaces. I can set the display to 1 or 2 decimals, and set the minimum to 0.0.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sounds good! Lets do it.

Comment 21

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - Endurance test support in mozmill-crowd

+ Adds decimal support to the UI delay preference
+ Renames entity strings based on above suggestions

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 520933 [details] [diff] [review]
Patch v3 - Endurance test support in mozmill-crowd

You forgot to place the textboxes into their own row.

Comment 23

[Aaron Train [:aaronmt]]

I didn't forget. I don't see the reason to  waste of space in my opinion. The position where they are now, http://i.imgur.com/8pASr.png works fine. General Tab in Firefox, example doesn't do that.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Well, we can keep it but then please fix the vertical alignment.

Comment 25

[Aaron Train [:aaronmt]]

Attachment: Patch v4 - Endurance test support in mozmill-crowd

+ Added spacers for alignment

Comment 26

[Aaron Train [:aaronmt]]

Attachment: Patch v4.1 - Endurance test support in mozmill-crowd

+ Bumped iteration count size to 6

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 521014 [details] [diff] [review]
Patch v4.1 - Endurance test support in mozmill-crowd

Looks better but as said in the last review please also fix the vertical alignment. The label and the textbox are not positioned at the same line.

Comment 28

[Aaron Train [:aaronmt]]

Attachment: Patch v5 - Endurance test support in mozmill-crowd

+ baselined element positioning

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 521165 [details] [diff] [review]
Patch v5 - Endurance test support in mozmill-crowd

Now I'm happy! That looks and works great. Lets get this in.

Comment 30

[Aaron Train [:aaronmt]]

Landed as https://github.com/whimboo/mozmill-crowd/commit/f25b6ce65bcab10d3d85a575c2898e002d527f22