676827 - Add preference to run endurance tests as non-restart tests

Status: RESOLVED FIXED

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

Description

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

It's possible for users of the command line automation to specify non-restart to disable restarting between each test. We should add this as a preference in Mozmill Crowd.

Comment 1

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

Attachment: Add preference to run endurance tests as non-restart tests. v1.0

Only requesting feedback for now as this is my first work on Mozmill Crowd, and I am having difficulties getting any version of Crowd to run locally at the moment.

Comment 2

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

Comment on attachment 551032 [details] [diff] [review]
Add preference to run endurance tests as non-restart tests. v1.0

>+        var restart = Utils.getPref("extensions.mozmill-crowd.endurance.restart", true);
[..]
>+
>+        if (!restart)
>+          args = args.concat("--no-restart");

I would prefer if we do not switch the meaning of the flag.

>+      <hbox align="baseline">
>+       <checkbox id="restart"
>+                label="&enduranceTestrunRestart.label;"
>+                accesskey="&enduranceTestrunRestart.accesskey;"
>+                preference="extensions.mozmill-crowd.endurance.restart"
>+                onsynctopreference=""/>

Why are you using 'onsynctopreference'?

Otherwise looks fine.

Comment 3

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

(In reply to Henrik Skupin (:whimboo) from comment #2)
> Comment on attachment 551032 [details] [diff] [review] [diff] [details] [review]
> Add preference to run endurance tests as non-restart tests. v1.0
> 
> >+        var restart = Utils.getPref("extensions.mozmill-crowd.endurance.restart", true);
> [..]
> >+
> >+        if (!restart)
> >+          args = args.concat("--no-restart");
> 
> I would prefer if we do not switch the meaning of the flag.

I understand and generally agree, however I feel that in this case it makes for a better user experience. Restarts occur by default, and a checked box to indicate this with the ability for a user to uncheck is a nicer design (imo) than having an unchecked box that when checked disables restarts.

> >+      <hbox align="baseline">
> >+       <checkbox id="restart"
> >+                label="&enduranceTestrunRestart.label;"
> >+                accesskey="&enduranceTestrunRestart.accesskey;"
> >+                preference="extensions.mozmill-crowd.endurance.restart"
> >+                onsynctopreference=""/>
> 
> Why are you using 'onsynctopreference'?

Copy/paste fail from the trustUnsecure checkbox -- does that even need it?

Comment 4

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

Attachment: Add preference to run endurance tests as non-restart tests. v1.1

Updated and tested successfully.

Comment 5

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

Comment on attachment 554029 [details] [diff] [review]
Add preference to run endurance tests as non-restart tests. v1.1

>+      <hbox align="baseline">
>+       <checkbox id="restart"
>+                label="&enduranceTestrunRestart.label;"
>+                accesskey="&enduranceTestrunRestart.accesskey;"
>+                preference="extensions.mozmill-crowd.endurance.restart"/>
>+      </hbox>

nit: seems like the indentation got off here.

>+<!ENTITY enduranceTestrunRestart.label       "Restart between each test">
>+<!ENTITY enduranceTestrunRestart.accesskey    "R">

nit: align both values to each other.

Otherwise I'm ok with your proposal re restart vs. no-restart. Looks good. Lets get the patch updated and I can check it in.

Comment 6

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

Attachment: Add preference to run endurance tests as non-restart tests. v1.2

(In reply to Henrik Skupin (:whimboo) from comment #5)
> Comment on attachment 554029 [details] [diff] [review]
> Add preference to run endurance tests as non-restart tests. v1.1
> 
> >+      <hbox align="baseline">
> >+       <checkbox id="restart"
> >+                label="&enduranceTestrunRestart.label;"
> >+                accesskey="&enduranceTestrunRestart.accesskey;"
> >+                preference="extensions.mozmill-crowd.endurance.restart"/>
> >+      </hbox>
> 
> nit: seems like the indentation got off here.

Done.

> >+<!ENTITY enduranceTestrunRestart.label       "Restart between each test">
> >+<!ENTITY enduranceTestrunRestart.accesskey    "R">
> 
> nit: align both values to each other.

Done.

Comment 7

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

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