930602 - refactor test.py definition of robocop tests so we don't repeat definitions of package

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

in test.py we have definitions (http://hg.mozilla.org/build/talos/file/tip/talos/test.py#l139) like:
am instrument -w -e deviceroot %s -e class org.mozilla.gecko.tests.testPan org.mozilla.roboexample.test/org.mozilla.gecko.FennecInstrumentationTestRunner

It would be nice to make talos just have the test name and package so we can substitute org.mozilla.gecko or org.mozilla.roboexample.test.

Comment 1

[Nick Alexander :nalexander [he/him]]

We'd really like to specify the test Android package (org.mozilla.roboexample.test) and test runner (org.mozilla.gecko.FennecInstrumentationTestRunner) in the tree, like we do for --fennec-IDs.  See

http://dxr.mozilla.org/mozilla-central/source/testing/talos/talos.json#l87

And it would be nice not to need to duplicate the declaration for every test in that JSON file -- but that would require adding a "default field" of some type to the JSON structure, which might be trickier.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

I am not too worried about the talos.json, but maybe we could define a default location for it in buildbot based automation so we don't need to define it in the talos.json.

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Joel Maher (:jmaher) from comment #2)
> I am not too worried about the talos.json, but maybe we could define a
> default location for it in buildbot based automation so we don't need to
> define it in the talos.json.

This defeats my intention: I don't want these defaults to be out of tree.  That's what perpetuates issues like Bug 930188 -- mxr doesn't show these hidden assumptions about the tree.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

these defaults could be in the talos tests.py file, and for the last 2 years this fennecids.txt file hasn't changed where it has been located or accessed by talos.

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Joel Maher (:jmaher) from comment #4)
> these defaults could be in the talos tests.py file, and for the last 2 years
> this fennecids.txt file hasn't changed where it has been located or accessed
> by talos.

From personal experience: I was too scared to move it when I was changing how robocop tests where built!

Comment 6

[Rishabh Roy]

hi i want to work on this bug  i am noob how can i test and get involved in this project

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Hi Rishabh- despite the back and forth in this bug, we can resolve a handful of the discussion fairly simply inside of talos.

1) setup talos: https://wiki.mozilla.org/Buildbot/Talos/Running#Running_locally_-_Source_Code
2) look at test.py, we will want to pass in two new variables on the commandline robocopTestName, robocopTestPackage
3) these will be used to replace values in then generated runtime object (look at uses of ${talos} for examples)

once we have this done, we can work on adding these fields to talos.json in the firefox tree and modifying the buildbot runners (scripts that call the talos toolchain) and glue it all together.

Thanks for working on this!

Comment 8

[Vaibhav (:vaibhav1994)]

Hi, I would like to take up this bug. I have talos running locally. Kindly give me some guidance to proceed on this bug.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

Hi Vaibhav, in Comment #7 I have outlined what needs to be done at a higher level.  Start by adding in the two new commandline options and validate internally that both are required (and that it is a robocop based test, as per bug 911302).

Then as outlined in step #3 above, you will need to edit the url line in the test.py to have placeholders, existing examples are ${talos}, ${firefox}.  You will see in the utils.py we replace these placeholders- the only key is that when calling this replacement function you will need to get the new values to that function.

Comment 10

[Vaibhav (:vaibhav1994)]

Attachment: refactor.patch

This patch refactors test.py definition of robocop tests so we don't repeat definitions of package

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8366218 [details] [diff] [review]
refactor.patch

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

I would like to see 2 more tests added:
1) that specifying both values produces a valid test['url']
2) that specifying no values produces the default values in test['url']

Great start!

::: talos/PerfConfigurator.py
@@ +514,5 @@
>          # add the tests to the configuration
>          # XXX extending vs over-writing?
>          self.config.setdefault('tests', []).extend(self.tests(activeTests, overrides, global_overrides, counters))
>  
> +        #test for ensuring that both robocopTestPackage and robocopTestName should be specified or else both should be None.

should be ''.  Also make it start like this: "# Test for ...", notice the space and the capitalized T

@@ +517,5 @@
>  
> +        #test for ensuring that both robocopTestPackage and robocopTestName should be specified or else both should be None.
> +        if (not self.config.get('robocopTestName','') and self.config.get('robocopTestPackage','')) or (self.config.get('robocopTestName','') and not self.config.get('robocopTestPackage','')):
> +            raise ConfigurationError('robocopTestName and robocopTestPackage must be specified together')
> +                

nit: this blank link has a lot of blank spaces/tabs, make it a blank line.

Please assign the default values here if we pass this test and the values are not ''

::: talos/utils.py
@@ +116,5 @@
>  def is_running(pid, psarg='axwww'):
>    """returns if a pid is running"""
>    return bool([i for i in mozpid.ps(psarg) if pid == int(i['PID'])])
>  
> +def interpolatePath(path, profile_dir=None, firefox_path=None,robocopTestPackage='org.mozilla.gecko',robocopTestName='org.mozilla.roboexample.test'):

I would rather put this logic in PerfConfigurator.py and make these default to None

@@ +121,2 @@
>    path = string.Template(path).safe_substitute(talos=here)
> +  

nit: make this a blank line

@@ +123,5 @@
>    if profile_dir:
>        path = string.Template(path).safe_substitute(profile=profile_dir)
>  
>    if firefox_path:
>        path = string.Template(path).safe_substitute(firefox=firefox_path)

I do not see where you substitute the values?!?

Comment 12

[Vaibhav (:vaibhav1994)]

Attachment: Cleaned up refactor.patch

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8367399 [details] [diff] [review]
Cleaned up refactor.patch

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

thanks for fixing this!

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

landed on talos:
https://hg.mozilla.org/build/talos/rev/4aa046fa2c7a

I am not going to close this bug as we need to patch the buildbot scripts to pass in these values.  We can revisit that once this is deployed to a few branches.

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

oops, we still would need to modify buildbot scripts to use this.

Comment 16

[Vaibhav (:vaibhav1994)]

Closing this bug as the talos part is over.