if talos has --fennecIDs specified on the commandline, ensure that it is a robocop based test

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jmaher, Assigned: vaibhav1994)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=jmaher][lang=python])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
if we use --fennecIDs on the command line, we set the profile up differently.  This isn't good and we do unnecessary actions.

we should check if fennecIDs is enabled that one of these tests are being used:
trobopan, tcheckerboard, tprovider, tcheck2

Ideally we would have an attribute on the tests in test.py so we could query the test attribute and the commandline flags!
(Reporter)

Updated

5 years ago
Whiteboard: [good first bug][mentor=jmaher][lang=python]
(Reporter)

Comment 1

5 years ago
This bug needs to have talos/test.py modified to have a true/false attribute on all tests, and if we are using robocop (a line similar to this):
http://hg.mozilla.org/build/talos/file/tip/talos/test.py#l138

you will add another attribute to the class:
fennecIDs = True

We can default it to False.

-------------

Once that is done, we need to modify the PerfConfigurator.py so that when we have --fennecIDs defined on the command line, but it is False on the test, we throw a configuration error.  This will take place in the validate function:
http://hg.mozilla.org/build/talos/file/tip/talos/PerfConfigurator.py#l358

you can test this by changing the commandline parameters to add --fennecIDs <path to file>, and testing various tests which do and do not support this argument.
(Assignee)

Comment 2

5 years ago
I would like to work on this bug. I have talos running locally. Kindly let me know how I have to proceed on this bug.
Assignee: nobody → vaibhavmagarwal
(Assignee)

Comment 3

5 years ago
Created attachment 8365140 [details] [diff] [review]
This patch ensures that when --fennecIDs is specified on the commandline, it is a robocop based test.
Attachment #8365140 - Flags: review?(jmaher)
(Reporter)

Comment 4

5 years ago
Comment on attachment 8365140 [details] [diff] [review]
This patch ensures that when --fennecIDs is specified on the commandline, it is a robocop based test.

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

close, a lot of little details to sort out- clean it up and I would love to review it again!

::: talos/PerfConfigurator.py
@@ +368,5 @@
>          self.config['remote'] = self.remote
>  
>          if self.remote:
>              # setup remote
> +                

please remove this extra whitespace added here

@@ +520,4 @@
>          for test in self.config.get('tests', []):
>              # robopan is the only robocop based extension which uses roboextender
>              if self.config.get('fennecIDs', '') and test['name'] == 'trobopan':
>                  self.config['extensions'] = ['${talos}/mobile_extensions/roboextender@mozilla.org']

you don't need to loop through the tests twice, just put this if condition inside the next for loop.

::: talos/ttest.py
@@ +216,5 @@
>              print "Remote Device Error: Error copying files for robocop setup"
>              raise
>  
>  
> +

please remove this extra line.

::: tests/test_PerfConfigurator.py
@@ +89,5 @@
>          error_tests = {'--activeTests':{'error':ConfigurationError,
>                                          'args':['--activeTests', 'badtest', '--develop', '-e', ffox_path, "-o", outfile],
>                                          'except_fault' : 'invalid --activeTest raised an error that is not ConfigurationError',
>                                          'non_raises_fault' : 'invalid --activeTest passed test'},
> +                       

please remove this extra line

@@ +91,5 @@
>                                          'except_fault' : 'invalid --activeTest raised an error that is not ConfigurationError',
>                                          'non_raises_fault' : 'invalid --activeTest passed test'},
> +                       
> +                       '--fennecIDs':{'error':ConfigurationError,
> +                                        'args':['--activeTests', 'ts', '--develop', '-e', ffox_path,'--fennecIDs',ffox_path,"-o", outfile],

nit: add whitespaces after ,'s to be consistent.

@@ +106,5 @@
> +                                        'except_fault' : 'invalid --fennecIDs raised an error that is not ConfigurationError',
> +                                        'non_raises_fault' : 'invalid --fennecIDs passed test'},
> +
> +                       
> +                       

nit, remove the extra whitepspace here, a blank line should be a blank line.

@@ +111,5 @@
>                         '--filter':{'error':ConfigurationError,
>                                     'args' : ['--activeTests', 'ts', '--develop',  '-e', ffox_path, '--filter', 'badfilter', '-o', outfile],
>                                     'except_fault' : 'invalid --filter raised an error that is not ConfigurationError',
>                                     'non_raises_fault' : 'invalid --filter passed test'},
> +                       

remove the whitespace here
Attachment #8365140 - Flags: review?(jmaher) → review-
(Assignee)

Comment 5

5 years ago
Created attachment 8365353 [details] [diff] [review]
Cleaned up fennecIDs.patch

This patch ensures that when --fennecIDs is specified on the commandline, it is a robocop based test.
Attachment #8365353 - Flags: review?(jmaher)
(Reporter)

Comment 6

5 years ago
Comment on attachment 8365353 [details] [diff] [review]
Cleaned up fennecIDs.patch

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

this looks great.  Do you want to commit this to the repository?
Attachment #8365353 - Flags: review?(jmaher) → review+
(Reporter)

Comment 7

5 years ago
thanks, I have landed this:
https://hg.mozilla.org/build/talos/rev/b2006ffcdf4c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.