Closed Bug 1123763 Opened 5 years ago Closed 5 years ago

[manifestparser] Implement a filter system for TestManifest.active_tests()

Categories

(Testing :: Mozbase, defect)

defect
Not set

Tracking

(firefox38 fixed)

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

As per bug 1054247, the end goal is to add various new filtering mechanisms for manipulating lists of tests, such as chunking, test runtimes/weights, labels and more.

Currently manifestparser has a variety of built-in "filters". I define a filter as a thing that takes a list of tests, manipulates it, and returns a new list of tests. The current filters are 'skip-if', 'run-if', 'fail-if', 'exists', 'disabled' and 'subsuite'.

These are all implemented haphazardly within the `active_tests` function in their own unique ways [1]. This won't scale.

This bug proposes formalizing the definition of a filter and making it easy to add new types of filters, whether built-in or user defined. The implementation of new filters (chunking, runtimes etc.) will be left to future bugs.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/manifestparser/manifestparser/manifestparser.py#757
Attached file MozReview Request: bz://1123763/ahal (obsolete) —
/r/2743 - Bug 1123763 - [manifestparser] Implement filtering system for TestManifest.active_tests

Pull down this commit:

hg pull review -r fa6cbf93bd6601d7dfb5d63f561d7d413796c6c5
/r/2743 - Bug 1123763 - [manifestparser] Implement filtering system for TestManifest.active_tests

Pull down this commit:

hg pull review -r 9e038b1b22492ff000266c7ec121eee013b30870
This adds a filters property to TestManifest. Looking at the documentation and test changes in the diff should give a pretty good idea on how this system works, but here is the gist of how adding a (future) chunking filter might look:

>    from manifestparser.filters import chunk
>    manifest.filters.add(chunk(this=1, total=5))
>    tests = manifest.active_tests()

Random notes:

* TestManifest.filters is an ordered set. This is the best data container since generally only one filter of each type should be applied, and the order filters get run in is actually important (i.e skip-if needs to come before disabled).

* The subsuite parameter is removed from the active_tests signature, but other than that everything else should be backwards compatible. I wanted to remove subsuite from active_tests while there was only one thing using it (mochitest is updated to use the new api).

* Did a major version bump to 1.0 (if this + bug 1120983 aren't enough to warrant a major bump, then I don't know what is).

* Green try runs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dae2e426c099
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3a684f99086
Attachment #8551953 - Flags: review?(wlachance)
/r/2743 - Bug 1123763 - [manifestparser] Implement filtering system for TestManifest.active_tests

Pull down this commit:

hg pull review -r 9e038b1b22492ff000266c7ec121eee013b30870
Attachment #8551953 - Flags: review?(wlachance) → review?(james)
/r/2743 - Bug 1123763 - [manifestparser] Implement filtering system for TestManifest.active_tests

Pull down this commit:

hg pull review -r 9e038b1b22492ff000266c7ec121eee013b30870
Attachment #8551953 - Flags: review?(james) → review?(ted)
/r/2743 - Bug 1123763 - [manifestparser] Implement filtering system for TestManifest.active_tests

Pull down this commit:

hg pull review -r 9e038b1b22492ff000266c7ec121eee013b30870
Blocks: chunking
Review ping?
Comment on attachment 8551953 [details]
MozReview Request: bz://1123763/ahal

https://reviewboard.mozilla.org/r/2741/#review2489

::: testing/mozbase/manifestparser/manifestparser/container.py
(Diff revision 2)
> +    overwrite one another.

This seems like non-intuitive behavior. Is there a reason you need this? It feels like it would probably be better to just error if you try to provide the same filter twice.

::: testing/mozbase/manifestparser/manifestparser/filters.py
(Diff revision 2)
> +def run_if(tests, values):

We're actually trying to remove run-if in bug 958147, just FYI.

::: testing/mozbase/manifestparser/manifestparser/filters.py
(Diff revision 2)
> +# You can obtain one at http://mozilla.org/MPL/2.0/.

This file could stand a comment describing filters like your paragraph in the docs. Maybe you can move that documentation here and just have it imported into the docs?

::: testing/mozbase/docs/manifestparser.rst
(Diff revision 2)
> +filters if the built-in ones are not enough.

I think it'd be nicer to define filters as accepting an iterable and returning an iterable so we can write them as generators if need be. Looking at the filters you've written I don't think this actually needs any code changes.

::: testing/mozbase/manifestparser/manifestparser/container.py
(Diff revision 2)
> +    an OrderedDict instead of a doubly linked list.

Is this correctly licensed based on that sample? (I hate code samples without licenses!)

::: testing/mozbase/manifestparser/manifestparser/filters.py
(Diff revision 2)
> +        return test

If you changed the definition of filters to be iterable -> iterable like I suggested above you could rewrite these as generators which might be nicer:
```
def skip_if(...):
  for test in tests:
    if whatever:
       dosomething
    yield test
```

::: testing/mozbase/manifestparser/manifestparser/filters.py
(Diff revision 2)
> +        return self._hash

Couldn't you just return `hash(self.__class__)` and skip the `__eq__`?

::: testing/mozbase/manifestparser/manifestparser/filters.py
(Diff revision 2)
> +        missing = self.mp.check_missing(tests)

I'm not sure I see the value in leaving the check_missing function in the ManifestParser vs. just moving the logic into here wholesale.

::: testing/mozbase/docs/manifestparser.rst
(Diff revision 2)
> +two convenience arguments:

It might be good to make this block of documentation generated from a docstring on the DEFAULT_FILTERS variable so that someone changing the list of default filters will be likely to keep the documentation in sync.

::: testing/mochitest/runtests.py
(Diff revision 2)
> +        tests = manifest.active_tests(disabled=disabled, **info)

I wonder if it wouldn't be nicer api-wise to just allow passing the filters into the active_tests call? You could just write:
```
tests = manifest.active_tests(disabled=disabled, filters=[subsuite(options.subsuite)], **info)
```

Did you have a use case in mind where we'd want to add filters to the Manifest object other than at the active_tests callsite?
Attachment #8551953 - Flags: review?(ted)
Thanks, excellent points! Reply in-line.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> This seems like non-intuitive behavior. Is there a reason you need this? It
> feels like it would probably be better to just error if you try to provide
> the same filter twice.

You're probably right. I was trying to account for filters where maybe you *might* want to apply them twice, but filters like that don't exist yet and error'ing out is more explicit.

 
> We're actually trying to remove run-if in bug 958147, just FYI.

Yeah, I wasn't sure when that was landing and didn't want to break anything unnecessarily.

  
> I think it'd be nicer to define filters as accepting an iterable and
> returning an iterable so we can write them as generators if need be. Looking
> at the filters you've written I don't think this actually needs any code
> changes.

Good idea, I think the subsuite filter won't work with generators, but should be fixable.


> Is this correctly licensed based on that sample? (I hate code samples
> without licenses!)

I don't know? I've made fairly substantial changes to it, I could just remove that comment I guess.


> I wonder if it wouldn't be nicer api-wise to just allow passing the filters
> into the active_tests call? You could just write:
> ```
> tests = manifest.active_tests(disabled=disabled,
> filters=[subsuite(options.subsuite)], **info)
> ```
> 
> Did you have a use case in mind where we'd want to add filters to the
> Manifest object other than at the active_tests callsite?

No, good idea, that's a bit nicer.

Patch upcoming.
https://reviewboard.mozilla.org/r/2741/#review2561

> Is this correctly licensed based on that sample? (I hate code samples without licenses!)

Since we're now raising on duplicate filters, might as well just use a list instead of an OrderedSet.

> Couldn't you just return `hash(self.__class__)` and skip the `__eq__`?

Switched container to a list, which doesn't use __hash__.

> I'm not sure I see the value in leaving the check_missing function in the ManifestParser vs. just moving the logic into here wholesale.

The main reason was that check_missing is supposed to raise if strict == True. Re-jiggered things around so that check_missing uses the exists filter.
Comment on attachment 8551953 [details]
MozReview Request: bz://1123763/ahal

/r/2743 - Bug 1123763 - [manifestparser] Implement filtering system for TestManifest.active_tests
/r/3171 - Address review comments; Use list instead of OrderedSet for underlying container

Pull down these commits:

hg pull review -r 8f4ea66155e5a43ba56bdf2bbfbb93ca0eee784e
Attachment #8551953 - Flags: review?(ted)
Comment on attachment 8551953 [details]
MozReview Request: bz://1123763/ahal

/r/2743 - Bug 1123763 - [manifestparser] Implement filtering system for TestManifest.active_tests
/r/3171 - Address review comments; Use list instead of OrderedSet for underlying container

Pull down these commits:

hg pull review -r 8f4ea66155e5a43ba56bdf2bbfbb93ca0eee784e
Comment on attachment 8551953 [details]
MozReview Request: bz://1123763/ahal

I seem to have broken mozreview. Unflagging until I can figure out what is going on.
Attachment #8551953 - Flags: review?(ted)
Comment on attachment 8551953 [details]
MozReview Request: bz://1123763/ahal

/r/2743 - Bug 1123763 - [manifestparser] Implement filtering system for TestManifest.active_tests
/r/3171 - Address review comments; Use list instead of OrderedSet for underlying container

Pull down these commits:

hg pull review -r 8f4ea66155e5a43ba56bdf2bbfbb93ca0eee784e
Attachment #8551953 - Flags: review?(ted)
Sigh, sorry for all the bugspam. Basically when I try to publish my review it will flip the bz flag to r? but then there's a 500 error and the change doesn't actually get published. (Filed mozreview bug 1127945)
I chatted with the mozreview team, and apparently a serious regression got in yesterday and my review request is more or less hosed. They don't have an eta for a fix, so recommended using splinter for the time being.

This is my follow-up commit (the interdiff) which addresses all of the previous review comments. Sorry for the inconvenience.
Attachment #8557249 - Flags: review?(ted)
Attachment #8551953 - Flags: review?(ted)
Comment on attachment 8557249 [details] [diff] [review]
Address review comments; Use list instead of OrderedSet

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

::: testing/mozbase/manifestparser/manifestparser/filters.py
@@ +29,3 @@
>          if tag in test and parse(test[tag], **values):
>              test.setdefault('disabled', '{}: {}'.format(tag, test[tag]))
> +        yield test

As I suspected, these are nicer to read as generators. Thanks!

@@ +80,5 @@
>  
>  class InstanceFilter(object):
>      """
>      Generally only one instance of a class filter should be applied at a time.
> +    Two instances of `InstanceFilter` are considered equal if they have thei

typo: 'thei'

@@ +140,5 @@
>  )
> +"""
> +By default :func:`~manifestparser.TestManifest.active_tests` will run the
> +:func:`~manifestparser.filters.skip_if`, :func:`~manifestparser.filters.run_if`
> +and :func:`~manifestparser.filters.fail_if` filters.

Boy, that Sphinx markup sure makes this hard for humans to read in source form.

@@ +144,5 @@
> +and :func:`~manifestparser.filters.fail_if` filters.
> +"""
> +
> +
> +class filterlist(MutableSequence):

I would think MutableSet would make a little more sense here, but it doesn't matter that much.

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py
@@ +745,1 @@
>              tests = fn(tests, values)

I was trying to find out if there was some more clever way to compose a pipeline of a set of generators, but I couldn't find anything. :)

@@ +746,1 @@
>          return tests

This probably needs to be list(tests) since we're using generators for the filters now.
Attachment #8557249 - Flags: review?(ted) → review+
(Thanks for providing the interdiff here since our tooling failed you!)
Thanks for the review!

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #18) 
> Boy, that Sphinx markup sure makes this hard for humans to read in source
> form.

Yeah :/, it adds nice links in the docs though. I can try to shorten it to :func:`~filters.fail_if`.


> I would think MutableSet would make a little more sense here, but it doesn't
> matter that much.

The problem is that order is important. E.g skip-if doesn't actually remove tests that are skipped, it just marks them with the disabled attribute. The disabled filter then removes any test that contains 'disabled', so it's important that disabled gets run after skip-if. My old OrderedSet implementation would have worked, but this way was less complicated.
Blocks: 1131098
Here's the full patch (initial mozreview + splinter interdiff + 2nd round of review comments addressed). Carrying r+ forward.

Here's a recent passing try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b79231a9265
Attachment #8551953 - Attachment is obsolete: true
Attachment #8557249 - Attachment is obsolete: true
Attachment #8561645 - Flags: review+
Sorry, I somehow managed to add a test from another patch to the manifest. On the upside, that push caught another bug where the error message lists every test except the missing one as missing.

Here's a new try run with the fix:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ecc21ea543c

Relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2717f1bd348
https://hg.mozilla.org/mozilla-central/rev/d2717f1bd348
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.