656393 - Restart between endurance tests by default

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Automation, defect)

Description

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

We want to restart the application between each endurance test, and allow the user to override this behaviour from the command line.

Comment 1

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

Attachment: Restart between endurance tests by default. v1

Comment 2

[u279076]

Comment on attachment 531697 [details] [diff] [review]
Restart between endurance tests by default. v1

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

Looks good to me, r+

Comment 3

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

Comment on attachment 531697 [details] [diff] [review]
Restart between endurance tests by default. v1

>         self.delay = "%.0d" % (delay * 1000)
>         self.iterations = iterations
>+        self.no_restart = no_restart

We should pass this option to the TestRun constructor instead of creating another class member.

Otherwise looks fine.

Comment 4

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

Attachment: Restart between endurance tests by default. v1.1

(In reply to comment #3)
> Comment on attachment 531697 [details] [diff] [review] [review]
> Restart between endurance tests by default. v1
> 
> >         self.delay = "%.0d" % (delay * 1000)
> >         self.iterations = iterations
> >+        self.no_restart = no_restart
> 
> We should pass this option to the TestRun constructor instead of creating
> another class member.

Done.

Comment 5

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

Comment on attachment 532971 [details] [diff] [review]
Restart between endurance tests by default. v1.1

>     def __init__(self, addons=None, application="firefox", binaries=None,
>-                 debug=False, logfile=None, report_url=None,
>+                 debug=False, logfile=None, no_restart=None, report_url=None,
>                  repository_path=None, repository_url=None,
>                  restart_tests=False, screenshot_path=None,
>                  test_path=None, timeout=None):

Sorry, seems like my last review comment wasn't clear enough. Please check the existent parameters of the constructor. You will find one called 'restart_tests'. It should perfectly fit the needs here, so we do not have to add another member to the testrun class, which is kinda confusing at this point.

>     def __init__(self, addons=None, binaries=None, delay=0.1, iterations=1,
>-                 logfile=None, report_url=None, repository_url=None,
>-                 screenshot_path=None):
>+                 logfile=None, no_restart=False, report_url=None,
>+                 repository_url=None, screenshot_path=None):

Which also affects the endurance constructor. Just negate the option from the command line.

Comment 6

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

Attachment: Restart between endurance tests by default. v1.2

(In reply to comment #5)
> Comment on attachment 532971 [details] [diff] [review] [review]
> Restart between endurance tests by default. v1.1
> 
> >     def __init__(self, addons=None, application="firefox", binaries=None,
> >-                 debug=False, logfile=None, report_url=None,
> >+                 debug=False, logfile=None, no_restart=None, report_url=None,
> >                  repository_path=None, repository_url=None,
> >                  restart_tests=False, screenshot_path=None,
> >                  test_path=None, timeout=None):
> 
> Sorry, seems like my last review comment wasn't clear enough. Please check
> the existent parameters of the constructor. You will find one called
> 'restart_tests'. It should perfectly fit the needs here, so we do not have
> to add another member to the testrun class, which is kinda confusing at this
> point.

Ah, sorry for missing that. Hopefully this patch is better.

> >     def __init__(self, addons=None, binaries=None, delay=0.1, iterations=1,
> >-                 logfile=None, report_url=None, repository_url=None,
> >-                 screenshot_path=None):
> >+                 logfile=None, no_restart=False, report_url=None,
> >+                 repository_url=None, screenshot_path=None):
> 
> Which also affects the endurance constructor. Just negate the option from
> the command line.

Done.

Comment 7

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

Attachment: Restart between endurance tests by default. v1.2

Forgot to qref.

Comment 8

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

Comment on attachment 533122 [details] [diff] [review]
Restart between endurance tests by default. v1.2

> class EnduranceTestRun(TestRun):
>     """ Class to execute a Firefox endurance test-run """
> 
>     report_type = "firefox-endurance"
> 
>     def __init__(self, addons=None, binaries=None, delay=0.1, iterations=1,
>-                 logfile=None, report_url=None, repository_url=None,
>-                 screenshot_path=None):
>+                 logfile=None, no_restart=False, report_url=None,
>+                 repository_url=None, screenshot_path=None):

We can also use restart_tests here and simply set the default to True. So we can get completely get rid of the no_restart parameter.

>+    parser.add_option("--no-restart",
>+                      action="store_true",
>+                      dest="no_restart",
>+                      default=False,
>+                      help="Do not restart application between tests")

If it would make clearer with the above proposed change we could name the option '--restart" and let it default to True. That way we don't have those double negative expressions which always causes confusion.

Otherwise it looks good. r=me with the above mentioned topics clarified.

Comment 9

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

Attachment: Restart between endurance tests by default. v1.3

(In reply to comment #8)
> Comment on attachment 533122 [details] [diff] [review] [review]
> Restart between endurance tests by default. v1.2
> 
> > class EnduranceTestRun(TestRun):
> >     """ Class to execute a Firefox endurance test-run """
> > 
> >     report_type = "firefox-endurance"
> > 
> >     def __init__(self, addons=None, binaries=None, delay=0.1, iterations=1,
> >-                 logfile=None, report_url=None, repository_url=None,
> >-                 screenshot_path=None):
> >+                 logfile=None, no_restart=False, report_url=None,
> >+                 repository_url=None, screenshot_path=None):
> 
> We can also use restart_tests here and simply set the default to True. So we
> can get completely get rid of the no_restart parameter.

Done.

> >+    parser.add_option("--no-restart",
> >+                      action="store_true",
> >+                      dest="no_restart",
> >+                      default=False,
> >+                      help="Do not restart application between tests")
> 
> If it would make clearer with the above proposed change we could name the
> option '--restart" and let it default to True. That way we don't have those
> double negative expressions which always causes confusion.
> 
> Otherwise it looks good. r=me with the above mentioned topics clarified.

Because of the way boolean parameters work I've kept --no-restart in, but this now sets restart_tests to False. By default this will be True. This resolves the double negatives in the TestRun class.

Comment 10

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

Comment on attachment 533480 [details] [diff] [review]
Restart between endurance tests by default. v1.3

Sweet. Works like a charm. Thanks Dave.

Comment 11

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

Landed as:
http://hg.mozilla.org/qa/mozmill-automation/rev/acede3ebc476