629077 - Write a new testrunner for the Endurance Tests

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

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

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.

Comment 1

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

Attachment: Testrunner script for endurance tests v1

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.

Comment 2

[u279076]

Comment on attachment 507192 [details] [diff] [review]
Testrunner script for endurance tests v1

Looks fine to me.

Comment 3

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

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.

Comment 4

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

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.

Comment 5

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

Attachment: Testrunner script for endurance tests v1.1

(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.

Comment 6

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

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.

Comment 7

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

Attachment: Testrunner script for endurance tests v1.2

(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

Comment 8

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

Attachment: Testrunner script for endurance tests v1.2.1

Removed the empty results from the persisted object. Patch for shared-module to follow momentarily.

Comment 9

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

Attachment: Updated shared-module for removal of empty results array in persisted object. v1.0

No longer reads results from persisted objects.

Comment 10

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

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.

Comment 11

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

Attachment: Updated shared-module for removal of empty results array in persisted object. v1.1

(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.

Comment 12

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

Attachment: Updated shared-module for removal of empty results array in persisted object. v1.2

Found that the reports were not including the results. Now fixed in this patch.

Comment 13

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

Attachment: Updated shared-module for removal of empty results array in persisted object. v1.2.1

Minor change to make comment style consistent.

Comment 14

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

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.

Comment 15

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

Attachment: Updated comments in shared-module v1.3 [checked-in]

(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.

Comment 16

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

Attachment: Testrunner script for endurance tests v1.3

Comment 17

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

Comment on attachment 513316 [details] [diff] [review]
Updated comments in shared-module v1.3 [checked-in]

Alright, lets work that way.

Comment 18

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

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

Comment 19

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

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.

Comment 20

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

Attachment: Testrunner script for endurance tests v1.3.1

(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.

Comment 21

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

Automation script for endurance tests has been landed:
http://hg.mozilla.org/qa/mozmill-automation/rev/3fc796baaf03