Closed Bug 629077 Opened 9 years ago Closed 9 years ago

Write a new testrunner for the Endurance Tests

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: davehunt, Assigned: davehunt)

References

Details

Attachments

(2 files, 9 obsolete files)

1.54 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
7.74 KB, patch
davehunt
: review+
Details | Diff | Splinter Review
We need to create a new testrunner to run the Endurance Tests. This will take an argument for the number of iterations the test snippets will be repeated, and include the metrics gathered during the test in the report.
This patch includes an additional testrunner for the endurance tests project, which takes two command line arguments: a delay in milliseconds to wait before starting each iteration, and the number of iterations to perform. The endurance results are added to the report. Currently the endurance results will be stored in memory, however it is intended that we will cache these on disk in the future.
Attachment #507192 - Flags: review?(hskupin)
Attachment #507192 - Flags: review?(anthony.s.hughes)
Attachment #507192 - Flags: feedback?(gmealer)
Comment on attachment 507192 [details] [diff] [review]
Testrunner script for endurance tests v1

Looks fine to me.
Attachment #507192 - Flags: review?(anthony.s.hughes) → review+
Comment on attachment 507192 [details] [diff] [review]
Testrunner script for endurance tests v1

Looks good to me from here. I'll admit that I'm not the most expert on the python wrappers, though, so I'll be interested to hear whimboo's feedback.
Attachment #507192 - Flags: feedback?(gmealer) → feedback+
Comment on attachment 507192 [details] [diff] [review]
Testrunner script for endurance tests v1

> class TestRun(object):
>     """ Class to execute a Mozmill test-run. """
> 
>     def __init__(self, addon_list=None, application="firefox", binaries=None,
>-                 debug=False, logfile=None, report_url=None,
>-                 repository_path=None, repository_url=None,
>+                 debug=False, delay=None, iterations=None, logfile=None,
>+                 report_url=None, repository_path=None, repository_url=None,

Looks like this change is not wanted and a left-over. Both properties have to be handled by your EnduranceTestRun class.

>+class EnduranceTestRun(TestRun):
[..]
>+    def __init__(self, binaries=None, delay=None, iterations=None,
>+                 logfile=None, report_url=None, repository_url=None):

Please define real standard values for delay (0) and iterations (1). None is not helpful and only requires additional work later.

>+    def prepare_tests(self):
>+        TestRun.prepare_tests(self)
>+        self._mozmill.persisted["delay"] = self.delay
>+        self._mozmill.persisted["iterations"] = self.iterations
>+        self._mozmill.persisted["endurance"] = []

I would be in favor of passing in everything inside the endurance property:

persisted['endurance'] = {delay: self.delay,
                          iterations: self.iterations,
                          results: [ ]}

I should update the UpdateTestRun to do the same.

>+    def run_tests(self):
[..]
>+        finally:
>+            self.results = self._mozmill.persisted["endurance"]

Why that finally?

>+    def update_report(self, report):
>+      TestRun.update_report(self, report)
>+
>+      report['endurance'] = self._mozmill.persisted['endurance']

... when you read the same _mozmill.persisted object here again for the real assignment?

Otherwise looks good, and given your reports on the dashboard it seems to work.
Attachment #507192 - Flags: review?(hskupin) → review-
Assignee: nobody → dave.hunt
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
(In reply to comment #4)
> Comment on attachment 507192 [details] [diff] [review]
> Testrunner script for endurance tests v1
> 
> > class TestRun(object):
> >     """ Class to execute a Mozmill test-run. """
> > 
> >     def __init__(self, addon_list=None, application="firefox", binaries=None,
> >-                 debug=False, logfile=None, report_url=None,
> >-                 repository_path=None, repository_url=None,
> >+                 debug=False, delay=None, iterations=None, logfile=None,
> >+                 report_url=None, repository_path=None, repository_url=None,
> 
> Looks like this change is not wanted and a left-over. Both properties have to
> be handled by your EnduranceTestRun class.

Correct. Removed.

> >+class EnduranceTestRun(TestRun):
> [..]
> >+    def __init__(self, binaries=None, delay=None, iterations=None,
> >+                 logfile=None, report_url=None, repository_url=None):
> 
> Please define real standard values for delay (0) and iterations (1). None is
> not helpful and only requires additional work later.

Done.

> >+    def prepare_tests(self):
> >+        TestRun.prepare_tests(self)
> >+        self._mozmill.persisted["delay"] = self.delay
> >+        self._mozmill.persisted["iterations"] = self.iterations
> >+        self._mozmill.persisted["endurance"] = []
> 
> I would be in favor of passing in everything inside the endurance property:
> 
> persisted['endurance'] = {delay: self.delay,
>                           iterations: self.iterations,
>                           results: [ ]}

Done.

> >+    def run_tests(self):
> [..]
> >+        finally:
> >+            self.results = self._mozmill.persisted["endurance"]
> 
> Why that finally?

Removed.
Attachment #507192 - Attachment is obsolete: true
Attachment #508374 - Flags: review?(hskupin)
Comment on attachment 508374 [details] [diff] [review]
Testrunner script for endurance tests v1.1

>+        self._mozmill.persisted['endurance'] = {'delay': self.delay,
>+                                                'iterations': self.iterations,
>+                                                'results': self.results}

Do we really have to set results here? I wonder how the endurance tests work if you wanna debug on test and the results property is not available. Can this last line be removed? That would also make the class member 'results' obsolete, which is also not necessary.

>+# Portions created by the Initial Developer are Copyright (C) 2010

2011 please.

>+base_path = os.path.dirname(os.path.abspath(__file__))
>+sys.path.append(os.path.join(base_path, 'libs'))
>+
>+from testrun import EnduranceTestRun

Importing of dependent modules has been already changed in the other scripts. Please update accordingly.

>+    parser.add_option("--delay",
>+                      default=0,
>+                      dest="delay",
>+                      metavar="INT",

Metavar should be "DELAY"

>+    parser.add_option("--iterations",
>+                      default=1,
>+                      dest="iterations",
>+                      metavar="INT",

Similar here.

Otherwise looks good. For the next cycle I will also execute a test-run. Looks like we can get this landed today.
Attachment #508374 - Flags: review?(hskupin) → review-
(In reply to comment #6)
> Comment on attachment 508374 [details] [diff] [review]
> Testrunner script for endurance tests v1.1
> 
> >+        self._mozmill.persisted['endurance'] = {'delay': self.delay,
> >+                                                'iterations': self.iterations,
> >+                                                'results': self.results}
> 
> Do we really have to set results here? I wonder how the endurance tests work if
> you wanna debug on test and the results property is not available. Can this
> last line be removed? That would also make the class member 'results' obsolete,
> which is also not necessary.

If I remove this the tests will fail. I'd need to first patch the endurance shared module so that it doesn't attempt to set the _results variable to the value of this. I have removed the class member 'results'.

> >+# Portions created by the Initial Developer are Copyright (C) 2010
> 
> 2011 please.

Done.

> >+base_path = os.path.dirname(os.path.abspath(__file__))
> >+sys.path.append(os.path.join(base_path, 'libs'))
> >+
> >+from testrun import EnduranceTestRun
> 
> Importing of dependent modules has been already changed in the other scripts.
> Please update accordingly.

Done.

> >+    parser.add_option("--delay",
> >+                      default=0,
> >+                      dest="delay",
> >+                      metavar="INT",
> 
> Metavar should be "DELAY"

Done.

> >+    parser.add_option("--iterations",
> >+                      default=1,
> >+                      dest="iterations",
> >+                      metavar="INT",
> 
> Similar here.

Set to ITERATIONS, however I'm not sure I like how this forced a new line when using --help
Attachment #508374 - Attachment is obsolete: true
Attachment #512789 - Flags: review?(hskupin)
Removed the empty results from the persisted object. Patch for shared-module to follow momentarily.
Attachment #512789 - Attachment is obsolete: true
Attachment #512906 - Flags: review?(hskupin)
Attachment #512789 - Flags: review?(hskupin)
No longer reads results from persisted objects.
Attachment #512907 - Flags: review?(hskupin)
Comment on attachment 512907 [details] [diff] [review]
Updated shared-module for removal of empty results array in persisted object. v1.0

>   if (persisted.endurance) {
>     this._delay = persisted.endurance.delay;
>     this._iterations = persisted.endurance.iterations;
>-    this._results = persisted.endurance.results;
>+    this._results = [];
>   } else {
>     //running the endurance test directly so set default values
>     this._delay = 0;
>     this._iterations = 1;
>-    this._results = [];
>   }

You will always have to initialize the results array. Looks like with the else case we will fail.
Attachment #512907 - Flags: review?(hskupin) → review-
(In reply to comment #10)
> Comment on attachment 512907 [details] [diff] [review]
> Updated shared-module for removal of empty results array in persisted object.
> v1.0
> 
> >   if (persisted.endurance) {
> >     this._delay = persisted.endurance.delay;
> >     this._iterations = persisted.endurance.iterations;
> >-    this._results = persisted.endurance.results;
> >+    this._results = [];
> >   } else {
> >     //running the endurance test directly so set default values
> >     this._delay = 0;
> >     this._iterations = 1;
> >-    this._results = [];
> >   }
> 
> You will always have to initialize the results array. Looks like with the else
> case we will fail.

Genuine mistake. Sorry.
Attachment #512907 - Attachment is obsolete: true
Attachment #513083 - Flags: review?(hskupin)
Found that the reports were not including the results. Now fixed in this patch.
Attachment #513083 - Attachment is obsolete: true
Attachment #513136 - Flags: review?(hskupin)
Attachment #513083 - Flags: review?(hskupin)
Minor change to make comment style consistent.
Attachment #513136 - Attachment is obsolete: true
Attachment #513166 - Flags: review?(hskupin)
Attachment #513136 - Flags: review?(hskupin)
Comment on attachment 513166 [details] [diff] [review]
Updated shared-module for removal of empty results array in persisted object. v1.2.1

>+  this._results = [];
>   if (persisted.endurance) {
>     this._delay = persisted.endurance.delay;
>     this._iterations = persisted.endurance.iterations;
>-    this._results = persisted.endurance.results;
>+    persisted.endurance.results = this._results;

The last line is a bit suspicious here, at least for the moment and as long as we don't write the results to the disk in the automation script. This change would reset the results array of the persisted object each time a new endurance test gets executed. So you will lose all the formerly collected information.

I would suggest that we have a short call later and can figure that out on the phone. Seems like we have to change a bit.
Attachment #513166 - Flags: review?(hskupin) → review-
(In reply to comment #14)
> Comment on attachment 513166 [details] [diff] [review]
> Updated shared-module for removal of empty results array in persisted object.
> v1.2.1
> 
> >+  this._results = [];
> >   if (persisted.endurance) {
> >     this._delay = persisted.endurance.delay;
> >     this._iterations = persisted.endurance.iterations;
> >-    this._results = persisted.endurance.results;
> >+    persisted.endurance.results = this._results;
> 
> The last line is a bit suspicious here, at least for the moment and as long as
> we don't write the results to the disk in the automation script. This change
> would reset the results array of the persisted object each time a new endurance
> test gets executed. So you will lose all the formerly collected information.
> 
> I would suggest that we have a short call later and can figure that out on the
> phone. Seems like we have to change a bit.

So this is the reason I needed the empty results array in the persisted object. There were several issues with removing this as suggested in comment 7. I have reverted this and tested locally.
Attachment #513166 - Attachment is obsolete: true
Attachment #513316 - Flags: review?(hskupin)
Attachment #512906 - Attachment is obsolete: true
Attachment #513317 - Flags: review?(hskupin)
Attachment #512906 - Flags: review?(hskupin)
Comment on attachment 513316 [details] [diff] [review]
Updated comments in shared-module v1.3 [checked-in]

Alright, lets work that way.
Attachment #513316 - Flags: review?(hskupin) → review+
Comment on attachment 513316 [details] [diff] [review]
Updated comments in shared-module v1.3 [checked-in]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/5de4abf1c862
Attachment #513316 - Attachment description: Updated shared-module for removal of empty results array in persisted object. v1.3 → Updated comments in shared-module v1.3 [checked-in]
Comment on attachment 513317 [details] [diff] [review]
Testrunner script for endurance tests v1.3

>+import os
>+import sys

Please remove both of those imports because they aren't necessary anymore. Then we are good to land.

r=me with that change.
Attachment #513317 - Flags: review?(hskupin) → review+
(In reply to comment #19)
> Comment on attachment 513317 [details] [diff] [review]
> Testrunner script for endurance tests v1.3
> 
> >+import os
> >+import sys
> 
> Please remove both of those imports because they aren't necessary anymore. Then
> we are good to land.
> 
> r=me with that change.

Done.
Attachment #513317 - Attachment is obsolete: true
Attachment #513426 - Flags: review+
Automation script for endurance tests has been landed:
http://hg.mozilla.org/qa/mozmill-automation/rev/3fc796baaf03
Status: ASSIGNED → RESOLVED
Closed: 9 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.