Closed
Bug 656393
Opened 13 years ago
Closed 13 years ago
Restart between endurance tests by default
Categories
(Mozilla QA Graveyard :: Mozmill Automation, defect)
Mozilla QA Graveyard
Mozmill Automation
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: davehunt, Assigned: davehunt)
References
Details
Attachments
(2 files, 3 obsolete files)
5.04 KB,
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
5.04 KB,
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
We want to restart the application between each endurance test, and allow the user to override this behaviour from the command line.
Assignee | ||
Comment 1•13 years ago
|
||
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+
Assignee | ||
Updated•13 years ago
|
Attachment #531697 -
Flags: review?(hskupin)
Comment 3•13 years ago
|
||
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-
Assignee | ||
Comment 4•13 years ago
|
||
(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 5•13 years ago
|
||
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-
Assignee | ||
Comment 6•13 years ago
|
||
(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)
Assignee | ||
Comment 7•13 years ago
|
||
Forgot to qref.
Attachment #533119 -
Attachment is obsolete: true
Attachment #533119 -
Flags: review?(hskupin)
Attachment #533122 -
Flags: review?(hskupin)
Comment 8•13 years ago
|
||
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+
Assignee | ||
Comment 9•13 years ago
|
||
(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 10•13 years ago
|
||
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+
Comment 11•13 years ago
|
||
Landed as: http://hg.mozilla.org/qa/mozmill-automation/rev/acede3ebc476
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•