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

RESOLVED FIXED

Status

Testing
Talos
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jmaher, Assigned: vaibhav1994)

Tracking

Trunk
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
Whiteboard: [good first bug][mentor=jmaher][lang=python]
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.
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.
(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.
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.
(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

4 years ago
hi i want to work on this bug  i am noob how can i test and get involved in this project
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!
(Assignee)

Comment 8

4 years ago
Hi, I would like to take up this bug. I have talos running locally. Kindly give me some guidance to proceed on this bug.
Assignee: nobody → vaibhavmagarwal
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.
(Assignee)

Comment 10

4 years ago
Created attachment 8366218 [details] [diff] [review]
refactor.patch

This patch refactors test.py definition of robocop tests so we don't repeat definitions of package
Attachment #8366218 - Flags: review?(jmaher)
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?!?
Attachment #8366218 - Flags: review?(jmaher) → review-
(Assignee)

Comment 12

4 years ago
Created attachment 8367399 [details] [diff] [review]
Cleaned up refactor.patch
Attachment #8367399 - Flags: review?(jmaher)
Comment on attachment 8367399 [details] [diff] [review]
Cleaned up refactor.patch

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

thanks for fixing this!
Attachment #8367399 - Flags: review?(jmaher) → review+
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.
Whiteboard: [good first bug][mentor=jmaher][lang=python]
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
oops, we still would need to modify buildbot scripts to use this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 16

4 years ago
Closing this bug as the talos part is over.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.