809262 - PerfConfigurator.py and filter.py currently dont check for validity of filter function names

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Jeff Hammel]

From

https://bug794292.bugzilla.mozilla.org/attachment.cgi?id=678925

+            options, args = example.parse_args(['--activeTests',
'ts', '--develop',  '-e',
+                                                ffox_path,
'--filter', 'badfilter', "-o", "test4.yaml"])

'badfilter' is never checked for and the test blindly passes

Comment 1

[Johannes Vogel (:nebelhom)]

looks like they are checking for it here ( http://hg.mozilla.org/build/talos/file/tip/talos/PerfConfigurator.py#l460 ), but it never catches anything...

Comment 2

[Jeff Hammel]

(In reply to nebelhom from comment #1)
> looks like they are checking for it here (
> http://hg.mozilla.org/build/talos/file/tip/talos/PerfConfigurator.py#l460 ),
> but it never catches anything...

filter.parse ( http://hg.mozilla.org/build/talos/file/tip/talos/PerfConfigurator.py#l459 , http://hg.mozilla.org/build/talos/file/tip/talos/filter.py#l116 ) currently doesn't ensure that the filter is in filter.filter_functions.  It probably should.

Comment 3

[Johannes Vogel (:nebelhom)]

Attachment: First Patch to this bug

Ok, it does what it is supposed to do (plus what we later discussed in IRC regarding scalar and series filters). I hope it's ok.

One minor detail, I would have to raise regarding the --filter help text in PerfConfigurator. It doesn't give an example on how to pass multiple filters (e.g. --filter ignore_max --filter median). I had to find an obscure website to find that out. Maybe give an example in the help text?

Comment 4

[Jeff Hammel]

Comment on attachment 680336 [details] [diff] [review]
First Patch to this bug

+                    if position == (len(_filters)-1):

Extraneous parentheses

This works for me.  The assertion errors should ideally have error messages, but this is better than what we have now.

:jmaher, any opinion on what tests should be run on try for checkin? I'm inclined to try: -b o -p win32,android -u none -t svgr,remote-tsvg

Comment 5

[Jeff Hammel]

(In reply to Johannes Vogel (:nebelhom) from comment #3)
<snip/>
> One minor detail, I would have to raise regarding the --filter help text in
> PerfConfigurator. It doesn't give an example on how to pass multiple filters
> (e.g. --filter ignore_max --filter median). I had to find an obscure website
> to find that out. Maybe give an example in the help text?

it would probably be better in general to document how to use filters. afaik, this is not done in the wiki, in the README, or in the help. filters are a complicated enough beast that something should be about them in the wiki or the README. There is also no explanation as to the difference between series and scalar filters or that the final filter must be a "scalar" filter.

Comment 6

[Johannes Vogel (:nebelhom)]

Attachment: First revision

Added assert error messages and removed those rather unpopular outer parentheses ;)

Comment 7

[Jeff Hammel]

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=39eb75bc2a11

Comment 8

[Mozilla RelEng Bot]

Try run for 39eb75bc2a11 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=39eb75bc2a11
Results (out of 5 total builds):
    success: 3
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-39eb75bc2a11

Comment 9

[Jeff Hammel]

Failure: 

Traceback (most recent call last):
  File "PerfConfigurator.py", line 866, in ?
    sys.exit(main())
  File "PerfConfigurator.py", line 858, in main
    options, args = conf.parse_args(args)
  File "PerfConfigurator.py", line 523, in parse_args
    options, args = Configuration.parse_args(self, *args, **kwargs)
  File "C:\talos-slave\talos-data\talos\configuration.py", line 476, in parse_args
    self(*config)
  File "PerfConfigurator.py", line 333, in __call__
    return Configuration.__call__(self, *args)
  File "C:\talos-slave\talos-data\talos\configuration.py", line 380, in __call__
    self.validate()
  File "PerfConfigurator.py", line 469, in validate
    raise ConfigurationError("Bad value for filter '%s': %s" % (filter_name, e))
__main__.ConfigurationError: Bad value for filter 'ignore_first:5': Any filter except the last has to be a series filter.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/build/talos/rev/5521b4717f6a

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Sorry, saw the whiteboard status and didn't notice comment 9 :(
https://hg.mozilla.org/build/talos/rev/339ee6afaa3e

Comment 12

[Jeff Hammel]

(In reply to Ryan VanderMeulen from comment #11)
> Sorry, saw the whiteboard status and didn't notice comment 9 :(
> https://hg.mozilla.org/build/talos/rev/339ee6afaa3e

Thanks for the backout. We've been using [talos-checkin-needed] to note patches that have passed review but have to be staged on try.  If desired, we could use a different whiteboard flag for that to avoid confusion.

Comment 13

[Johannes Vogel (:nebelhom)]

hmmm... interesting what a small change in order can do...

I fixed that particular failure, but on running the tests for filter, one of the tests stumbles over my newly added assert statement. Here is the read_out:

..F
======================================================================
FAIL: test_parse (__main__.TestFilter)
test the filter name parse function
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_filter.py", line 64, in test_parse
    parsed = talos.filter.parse('foo:10.1,2,5.0,6.')
  File "/home/nebelhom/Mozilla-Dev/talos/talos/filter.py", line 144, in parse
    "--filter value not found in filters."
AssertionError: --filter value not found in filters.

----------------------------------------------------------------------
Ran 3 tests in 0.001s

FAILED (failures=1)

Should I adjust that particular test, as well, so that it expects an AssertionError? Should I include this example in test_PerfConfigurator.py, as well?

I am asking, because I am not familiar what the initial thinking behind the filter.parse() function was. Is it supposed to just blindly and mechanically make out of 'filter:arg' a tuple ('filter, arg) without any qualitative checks? If that is the case, any other use of filter.parse in talos may throw such an unexpected exception. As a consequence, I would remove the assert statement from filter.parse() and move it into PerfConfigurator.py

What do you guys think?

Comment 14

[Jeff Hammel]

The behaviour of filter.parse() hitherto was a blind and mechanical return of (filter, [args]).   Since changing it breaks the test, but the test is correct outside of not having a 'foo' function, the test should be fixed.

Since we don't have any filter functions currently that take more than one argument, I would do the following:

1. have the test add a function 'foo' to the filter functions in filter.py . This function should take an arbitrary number of arguments
2. (Keep the rest of the test the same)
3. Remove the function from the filter.py filter functions following the test

Comment 15

[Johannes Vogel (:nebelhom)]

Attachment: Next Revision

Ok, here is the next try.

I adjusted the test_filter.py to add the key/value pair 'foo':lambda *args:args to the scalar_filters dictionary.

I tried to make sure that it mimics a real case as closely as possible, so that no other funky side effects in other parts may happen (in scalar as it is the only filter, lambda function etc.)

I hope that cuts it. thanks for checking.

Comment 16

[Jeff Hammel]

Comment on attachment 681732 [details] [diff] [review]
Next Revision

looks good to me!

Comment 17

[Jeff Hammel]

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=fd17214198be

Comment 18

[Mozilla RelEng Bot]

Try run for fd17214198be is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=fd17214198be
Results (out of 3 total builds):
    success: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fd17214198be

Comment 19

[Jeff Hammel]

pushed: http://hg.mozilla.org/build/talos/rev/9146bb55321c