write a tests/talostest.py to autophone for tp4m/tsvg

RESOLVED FIXED

Status

Testing
Autophone
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jmaher, Assigned: jmaher)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
We need a master script (most likely similar to s1s2test).

This would:
* create the manifest.develop dynamically
* build the url to pass in
* set pageloader.xpi + quitter.xpi in the profile creation
* ensure we have the right preferences
* write a custom log parser to parse the logcat and get the results
(Assignee)

Updated

3 years ago
Depends on: 1172106
(Assignee)

Comment 1

3 years ago
Created attachment 8616184 [details]
prototype talostest.py (0.2)
(Assignee)

Updated

3 years ago
Depends on: 1172121
(Assignee)

Comment 2

2 years ago
Created attachment 8634103 [details]
talostest.py - current version (0.5)
Assignee: nobody → jmaher
Attachment #8616184 - Attachment is obsolete: true
Status: NEW → ASSIGNED

Comment 3

2 years ago
It is unclear to me how we will proceed here. Will you be reporting to phonedash? If not, is inheriting from S1S2Test appropriate or would it be better to just inherit from PerfTest and customize for your needs without having to modify S1S2Test?

I think I would like to meet up and talk about this in person when you get back.
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1172104
(Assignee)

Comment 5

2 years ago
we need to update ep1:
https://git.mozilla.org/?p=automation/ep1.git;a=commit;h=579fa0b401f717f6449cb3ce656b3ed2746d1629

Comment 6

2 years ago
and update Autophone's ep1 Subproject
https://github.com/mozilla/autophone/commit/9c12f897833a713c80954ba0b003c2195dc1bc53
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1172121
(Assignee)

Comment 8

2 years ago
we might need to adjust our ep1/talos/*/*.manifest files as it would be nice to not assume a flat file structure, but overall we are close.

Here is a runner script (with no shared code):
https://github.com/jmaher/autophone/blob/talos/tests/talostest.py

Ideally we will do some basic refactoring to remove obvious code duplication, fix ep1/talos manifests, and then sort out the configs/pageloader-[remote|local].ini files to define tp4m and tsvg.
(Assignee)

Comment 9

2 years ago
Created attachment 8639463 [details] [diff] [review]
initial work for talostest (1.0)

this is working locally for me and for bc, lets make it formal.
Attachment #8634103 - Attachment is obsolete: true
Attachment #8639463 - Flags: review?(bob)

Comment 10

2 years ago
Comment on attachment 8639463 [details] [diff] [review]
initial work for talostest (1.0)

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

Unfortunately, due to the way PhoneTest and PerfTest were designed you had to reimplement create_profile, wait_for_fennec, install_local_pages rather than reuse them. I will take care of that in addition to dealing with the duplication of code in the TalosTest init method.

r- for the reasons mentioned below. Work up a new version of the patch and test it and I will review it asap.

::: configs/pageloader-local.ini
@@ +12,5 @@
> +
> +[settings]
> +tpargs = -tpnoisy -rss -tpcycles 4 -tppagecycles 1 -tpdelay 1000
> +
> +iterations = 1

Since you are not using these stderrp_* values, you can remove them from your ini file complete.
== begin delete ==

@@ +23,5 @@
> +stderrp_reject = 10
> +
> +# The iterations for a test are retried up to
> +# stderrp_attempts if they are rejected.
> +stderrp_attempts = 2

== end delete ==

::: configs/pageloader-remote.ini
@@ +1,2 @@
> +[paths]
> +sources = files/base/ files/ep1/talos/tp4m/ files/ep1/talos/tsvg/

Since this is remote, we probably only need the base. But lets leave it in for now and if needed we can fix up when we deal with the url paths on the sdcard and phonedash server.

@@ +12,5 @@
> +
> +[settings]
> +tpargs = -tpnoisy -rss -tpcycles 4 -tppagecycles 1 -tpdelay 1000
> +
> +iterations = 1

Since you are not using these stderrp_* values, you can remove them from your ini file complete.
== begin delete ==

@@ +23,5 @@
> +stderrp_reject = 10
> +
> +# The iterations for a test are retried up to
> +# stderrp_attempts if they are rejected.
> +stderrp_attempts = 2

== end delete ==

::: configs/tcheck-remote.ini
@@ +13,5 @@
> +[settings]
> +tcheck_args = -e class org.mozilla.gecko.tests.testCheck2 org.mozilla.roboexample.test/org.mozilla.gecko.FennecInstrumentationTestRunner
> +
> +
> +iterations = 5

Since you are not using these stderrp_* values, you can remove them from your ini file complete.
== begin delete ==

@@ +24,5 @@
> +stderrp_reject = 10
> +
> +# The iterations for a test are retried up to
> +# stderrp_attempts if they are rejected.
> +stderrp_attempts = 1

== end delete ==

::: files/ep1
@@ -1,1 @@
> -Subproject commit 059a91048a94cf274d71a180814d400c9b05bc68

If you checkout your master branch, pull from mozilla/autophone upstream, then checkout your talos branch and then rebase against your master you will find this commit is already on the master branch and will disappear from your patch.

::: phonetest.py
@@ +647,1 @@
>          self.loggerdeco.debug('run_fennec_with_profile: %s %s' % (appname, url))

Go ahead and include the extra_args in the debug message...

self.loggerdeco.debug('run_fennec_with_profile: %s %s %s' % (appname, url, extra_args))

::: tests/talostest.py
@@ +40,5 @@
> +def geometric_mean(series):
> +    """
> +    geometric_mean: http://en.wikipedia.org/wiki/Geometric_mean
> +    """
> +    # bc HACK

Remove this comment.

@@ +47,5 @@
> +    total = 0
> +    for i in series:
> +        total += math.log(i+1)
> +    return math.exp(total / len(series)) - 1
> +

median and geometric_mean seem pretty useful in general and could be used outside of this file. Let's move them both to utils.py and import them from utils.py for use in this file.

@@ +53,5 @@
> +class TalosTest(PerfTest):
> +    def __init__(self, dm=None, phone=None, options=None,
> +                 config_file=None, chunk=1, repos=[]):
> +
> +        self.enable_unittests = False

This is redundant as it is set in PhoneTest to be False. You only need to over-ride it if you want to change it to True.

@@ +104,5 @@
> +                self._tests[t[0]] = t[1]
> +        except ConfigParser.NoSectionError:
> +            self._tests['blank'] = 'blank.html'
> +        # Map URLS - {urlname: url} - urlname serves as testname
> +        self._urls = {}

The way you are using self._urls seems potentially confusing. These aren't urls in the normal sense but are extra arguments to the run_fennec_with_profile method. Would you be ok with something like:

# Map test names to test arguments - {testname: args}
# where args is a space separated list of extra arguments
# to run_fennec_with_profile

self._test_args = {}

@@ +107,5 @@
> +        # Map URLS - {urlname: url} - urlname serves as testname
> +        self._urls = {}
> +        config_vars = {'webserver_url': options.webserver_url}
> +
> +        autophone_directory = os.path.dirname(os.path.abspath(sys.argv[0]))

This is already set above to an identical value.

@@ +113,5 @@
> +            location_items = self.cfg.items('locations', False, config_vars)
> +        except ConfigParser.NoSectionError:
> +            location_items = [('local', None)]
> +
> +        for test_location, test_path in location_items:

S1S2Test had a check for the pseudo-options (the attribute names from the config_vars) here

            if test_location in config_vars:
                # Ignore the pseudo-options which result from passing
                # the config_vars for interpolation.
                continue

which you have removed but appear to work around by checking that test_location can only be either 'remote' or 'local' below.

@@ +119,5 @@
> +                tpname = self._tests[test_name]
> +                tpargs = self.cfg.get('settings', 'tpargs')
> +                manifest_root = "file:%s" % self._paths['dest']
> +                tppath = os.path.join(manifest_root, tpname)
> +                test_url = "-tp %s.develop %s" % (tppath, tpargs)

extra_args = "-tp %s.develop %s" % (tppath, tpargs)

@@ +122,5 @@
> +                tppath = os.path.join(manifest_root, tpname)
> +                test_url = "-tp %s.develop %s" % (tppath, tpargs)
> +
> +                if test_location not in ['remote', 'local']:
> +                    continue

This would have been taken care of by the check for the pseudo options. But if you keep this, it should be before the |for test_name| loop since you are doing unnecessary work if the test_location is not 'remote' or 'local'.

@@ +135,5 @@
> +                        with open(fname, 'r') as fHandle:
> +                            lines = fHandle.readlines()
> +
> +                        if test_location == 'remote':
> +                            manifest_root = config_vars['webserver_url']

config_vars was really just to get the interpolation from the config file. You can use options.webserver_url here without using config_vars at all.

@@ +145,5 @@
> +                    pass
> +
> +                self.loggerdeco.debug(
> +                    'test_location: %s, test_name: %s, test_path: %s, '
> +                    'test: %s, test_url: %s' %

test_url -> extra_args

@@ +147,5 @@
> +                self.loggerdeco.debug(
> +                    'test_location: %s, test_name: %s, test_path: %s, '
> +                    'test: %s, test_url: %s' %
> +                    (test_location, test_name, test_path,
> +                     self._tests[test_name], test_url))

test_url -> extra_args

@@ +148,5 @@
> +                    'test_location: %s, test_name: %s, test_path: %s, '
> +                    'test: %s, test_url: %s' %
> +                    (test_location, test_name, test_path,
> +                     self._tests[test_name], test_url))
> +                self._urls["%s-%s" % (test_location, test_name)] = test_url

self._test_args["%s-%s" % (test_location, test_name)] = extra_args

@@ +154,5 @@
> +                                            'initialize_profile.html')
> +
> +    @property
> +    def type(self):
> +        return 'talos'

I'm not real thrilled about this, but it is really fall out due to the fact that several methods should have been part of PhoneTest and been reusable instead of being squirreled away in S1S2Test.

Don't worry about it for now, but this is something I will deal with in the refactoring we discussed.

@@ +178,5 @@
> +                PhoneTestResult.BUSTED)
> +            return is_test_completed
> +
> +        is_test_completed = True
> +        testcount = len(self._urls.keys())

self._urls -> self._test_args

@@ +179,5 @@
> +            return is_test_completed
> +
> +        is_test_completed = True
> +        testcount = len(self._urls.keys())
> +        for testnum, (testname, url) in enumerate(self._urls.iteritems(), 1):

for testnum, (testname, extra_args) in enumerate(self._test_args.iteritems(), 1):

@@ +198,5 @@
> +            # typically due to a regression in the brower which should
> +            # be reported.
> +            success = False
> +            command = None
> +            for attempt in range(1, self.stderrp_attempts+1):

If you aren't using the stderrp_* stuff, no need for this loop.

@@ +211,5 @@
> +                #
> +                # It is possible for an item in the dataset to have an
> +                # uncached value and not have a corresponding cached
> +                # value if the cached test failed to record the
> +                # values.

You aren't doing the cached vs uncached stuff, so remove this paragraph about cached vs uncached.

@@ +216,5 @@
> +
> +                dataset = []
> +                for iteration in range(1, self._iterations+1):
> +                    command = self.worker_subprocess.process_autophone_cmd(
> +                        test=self, require_ip_address=url.startswith('http'))

Here is where the url confusion really bites us. In your case url is not something that begins with file: or http: but is the extra argument list. You need to check the test name to see if it begins with remote in order to determine if an ip address is needed.

command = self.worker_subprocess.process_autophone_cmd(
   test=self, require_ip_address=testname.startswith('remote'))

@@ +228,5 @@
> +
> +                    self.update_status(message='Attempt %d/%d for Test %d/%d, '
> +                                       'run %d, for url %s' %
> +                                       (attempt, self.stderrp_attempts,
> +                                        testnum, testcount, iteration, url))

no stderrp_* stuff means you can rewrite this message and remove the attempt/stderrp_attempts part of it.

@@ +239,5 @@
> +                                          'Failed to create profile',
> +                                          PhoneTestResult.TESTFAILED)
> +                        continue
> +
> +                    #NOTE: no uncached vs cached- pageloader accounts for this.

Remove this comment.

@@ +240,5 @@
> +                                          PhoneTestResult.TESTFAILED)
> +                        continue
> +
> +                    #NOTE: no uncached vs cached- pageloader accounts for this.
> +                    measurement = self.runtest(url)

measurement = self.runtest(extra_args)

@@ +285,5 @@
> +                                      'No measurements detected.',
> +                                      PhoneTestResult.BUSTED)
> +                    break
> +
> +                #NOTE: ignoring is_stderr_below_threshold and rejected status

If we are ignoring the stderr checks then we can safely leave the stderrp_* values out of the pageloader-{local,remote}.ini files. If they are missing they will be set to appropriate default values in PerfTest's init.
Go ahead and remove this comment.

@@ +301,5 @@
> +                break
> +
> +        return is_test_completed
> +
> +    def runtest(self, url):

def runtest(self, extra_args):

@@ +307,5 @@
> +        self.logcat.clear()
> +
> +        # Run test
> +        self.run_fennec_with_profile(self.build.app_name, "",
> +                                     extra_args=url.split(' '))

url -> extra_args

@@ +315,5 @@
> +        pageload_metric = self.analyze_logcat()
> +
> +        # Ensure we succeeded - no 0's reported
> +        datapoint = {'pageload_metric': pageload_metric}
> +        return datapoint

If pageload_metric['summary'] == 0, then we have no measurement, right? You don't ensure here that no 0s are reported and always return a non-empty object which means in your run_job method you will never detect a missing measurement.

Following S1S2Test's approach, I think 

        datapoint = {}
        if pageload_metric['summary'] != 0:
            datapoint = {'pageload_metric': pageload_metric}
        return datapoint

@@ +437,5 @@
> +        max_attempts = max_time / wait_time
> +
> +        results = {}
> +        pageload_metric = {'summary': 0}
> +        while (attempt <= max_attempts and pageload_metric['summary'] == 0):

redundant parens

@@ +441,5 @@
> +        while (attempt <= max_attempts and pageload_metric['summary'] == 0):
> +            buf = self.logcat.get()
> +            for line in buf:
> +                self.loggerdeco.debug('analyze_logcat: %s' % line)
> +                match = re_page_data.match(line)

This line should be moved to just before the |if match:| below.

@@ +443,5 @@
> +            for line in buf:
> +                self.loggerdeco.debug('analyze_logcat: %s' % line)
> +                match = re_page_data.match(line)
> +                end = re_end_report.match(line)
> +                if end:

No need to introduce a new variable. We can just re-use match as in:

match = re_end_report.match(line)
if match:

@@ +449,5 @@
> +                    data = []
> +                    for page in results:
> +                        data.append(median(results[page]))
> +                        pageload_metric[page] = median(results[page])
> +                    pageload_metric['summary'] = geometric_mean(data)

Just an observation: The fact that I had to introduce a check for a zero length argument to geometric_mean means, that there was a circumstance in my testing where __end_tp_report was detected but no results were saved. It may have been due to our known potential issues with paths to the test files or something else but it was possible.

@@ +463,5 @@
> +
> +            if self.fennec_crashed:
> +                # If fennec crashed, don't bother looking for pageload metric
> +                break
> +            if (pageload_metric['summary'] == 0):

redundant parens

@@ +466,5 @@
> +                break
> +            if (pageload_metric['summary'] == 0):
> +                sleep(wait_time)
> +                attempt += 1
> +        if pageload_metric['summary'] == 0:

If pageload_metric['summary'] == 0 then we have failed to detect any measurements, right?

@@ +469,5 @@
> +                attempt += 1
> +        if pageload_metric['summary'] == 0:
> +            self.loggerdeco.info('Unable to find pageload metric')
> +
> +        return (pageload_metric)

redundant parens.
Attachment #8639463 - Flags: review?(bob) → review-
(Assignee)

Comment 11

2 years ago
Created attachment 8642157 [details] [diff] [review]
initial work for talostest (2.0)

this is updated with all the review feedback!
Attachment #8639463 - Attachment is obsolete: true
Attachment #8642157 - Flags: review?(bob)

Comment 12

2 years ago
Comment on attachment 8642157 [details] [diff] [review]
initial work for talostest (2.0)

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

I'll deal with the use of the type property and the destinations on the sdcard in the refactor.

::: configs/tcheck-remote.ini
@@ +31,5 @@
> +job_name = Autophone Rck2
> +job_symbol = rck2
> +group_name = Autophone
> +group_symbol = A
> +

Let's not check this in until you have the tcheck test written.

::: configs/tsvg-remote.ini
@@ +16,5 @@
> +iterations = 1
> +
> +[treeherder]
> +job_name = Autophone Tsvg
> +job_symbol = s

Let's change this to svg since it conflicts with the Smoketest job_symbol.

::: tests/talostest.py
@@ +84,5 @@
> +            location_items = [('local', None)]
> +
> +        for test_location, test_path in location_items:
> +            if test_location not in ['remote']:
> +                continue

If we ever wished to enable local tests we would have to modify this test. The solution in https://github.com/mozilla/autophone/blob/master/tests/s1s2test.py#L86 will work to exclude the pseudo option name created as a result of the interpolation. Please change this and test to make sure it works with both local and remote options even if you only commit the remote config file.
Attachment #8642157 - Flags: review?(bob) → review+

Updated

2 years ago
Blocks: 1190912
(Assignee)

Comment 13

2 years ago
thanks, I have addressed the feedback, tested locally and committed:
https://github.com/mozilla/autophone/commit/1fa6160587741cb3a8789614c481397a6c0f3ec2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.