Closed Bug 656393 Opened 8 years ago Closed 8 years ago

Restart between endurance tests by default

Categories

(Mozilla QA Graveyard :: Mozmill Automation, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: davehunt, Assigned: davehunt)

References

Details

Attachments

(2 files, 3 obsolete files)

We want to restart the application between each endurance test, and allow the user to override this behaviour from the command line.
Assignee: nobody → dave.hunt
Attachment #531697 - Flags: review?(anthony.s.hughes)
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+
Attachment #531697 - Flags: review?(anthony.s.hughes) → review+
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.
Attachment #531697 - Flags: review?(hskupin) → review-
(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.
Attachment #531697 - Attachment is obsolete: true
Attachment #532971 - Flags: feedback?(hskupin)
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.
Attachment #532971 - Flags: feedback?(hskupin) → feedback-
(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.
Attachment #532971 - Attachment is obsolete: true
Attachment #533119 - Flags: review?(hskupin)
Forgot to qref.
Attachment #533119 - Attachment is obsolete: true
Attachment #533119 - Flags: review?(hskupin)
Attachment #533122 - Flags: review?(hskupin)
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.
Attachment #533122 - Flags: review?(hskupin) → review+
(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.
Attachment #533480 - Flags: review?(hskupin)
Comment on attachment 533480 [details] [diff] [review]
Restart between endurance tests by default. v1.3

Sweet. Works like a charm. Thanks Dave.
Attachment #533480 - Flags: review?(hskupin) → review+
Landed as:
http://hg.mozilla.org/qa/mozmill-automation/rev/acede3ebc476
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.