Improve ErrorReportingRunner, make it support Filterable, accept a class name and support better test names

[paulduffin]

ErrorReportingRunner does not support Filterable which means it doesn't play well with Filters that are trying to remove known issues. Filters often ignore descriptions that have children so that they do not filter out whole suites of tests and instead rely on a Runner that has children to support Filterable. ErrorReportingRunner has children but doesn't support Filterable so it is impossible to exclude specific problems.

All its children (in its Description) are called initializationError, that makes it impossible to filter out specific error messages.

It expects a Class<?> as its parameter which means that it cannot be used to handle exceptions resulting from a failure to load a class. It should also take a String parameter.

[kcooney]

I'm not sure ErrorReportingRunner should report filterable. It's usually used to report critical errors during test initialization. If it were filterable, it would be too easy for errors to be unintentionally filtered out.

Having all the children called initializationError is problematic. I think 0e4cb7e fixed one of the major causes of this.

Can you give an example of the kind of known issue that you would want to filter out?

[paulduffin]

ErrorReportingRunner not support Filterable doesn't make it impossible to filter out it just makes it a lot more awkward as filter implementations have to handle it specially. That adds maintenance and development costs, and increases the cost of upgrades as the class is internal and has changed quite a few times.

In AOSP tests are run in many different environments and in many different ways, some of the differences can cause test classes to fail to load, e.g. a test requires a class that is not visible or present on the Android version which causes the validation to fail.

While we could possibly address this by modifying those classes to make them more flexible that is more costly to develop and maintain and more risky than using general filter mechanisms that apply across all tests. This is especially true where the tests are sourced from an upstream source so fixing the issue requires patching locally within AOSP and then preserving that patch across upgrades.

Often times while porting something to work on Android we will temporarily disable tests by filtering out those that are broken (using @ignore doesn't work when the class can't even be loaded). That allows us to submit the changes and fix the broken parts gradually rather than wait for all the problems to be fixed before submitting.

[kcooney]

Currently, the only case where ErrorReportingRunner will create a Description with more than one child is when you use Request.classes(). You can avoid using this method by creating an on-the-fly Suite that contains the classes you want to run, and then using Request.runner()

Even though the runner isn't Filterable, you should still be able to filter out errors because any runner that extends ParentRunner will call Filter.shouldRun() on its children.

I don't think most filters should ignore descriptions with children. In fact, most filters should only remove a parent if all children were filtered. Based on another issue, I have considered adding a subclass of Filter that makes it easier to do this.

I see no harm in adding a constructor that takes in a class name (instead of a class) but please note that ErrorReportingRunner is an internal class. The public API is Request.errorReport(), so you would need to add a method there, too

[paulduffin]

Actually, ErrorReportingRunner will create a Description with more than one child any time that ParentRunner.validate() finds more than one error, e.g. two @test annotated methods with a parameter. However, for the purposes of this discussion it really doesn't matter whether there is one child or many.

When I said a Filter ignores a Description with children I meant that the filters we have (which allow filtering by package, class or method) don't filter out Descriptions that have children unless they end up with no children. That is to avoid a less specific filter rule from overriding a more specific rule.

The maximal set of tests that should be run are specified by the user, outside the influence of our filtering rules. All our filtering rules can do is remove any unwanted tests from that set, it cannot add ones that aren't already in the set. In order to ensure maximum flexibility we can filter by package, class or method and the most specific wins, i.e. method wins over class wins over package.

Say, we only want to run one method in a class that has fifty methods, we have to first disable the class and then enable the method, e.g. something like this, !package.class, package.class#method7.

The Description would look like:

• package.class
  • package.class#method1
  • package.class#method2
  • ...
  • package.class#method7
  • ...
  • package.class#method50

If we simply applied our rules to the top level Description(package.class) we would disable the children. We could rewrite our data structures so that we would know at the point we tried to filter the top level class Description that we had a more specific rule for a method in that class. That would work for this case but not other cases.

e.q. It wouldn't work for say test suites that include classes from packages that are outside their 'natural' hierarchy, e.g. if we had a package1.package2.testsuite that included say package2.package3.class and a rule something like !package1, package2 then we would ignore package1.package2.testsuite and also package2.package3.class. That would make the rules sensitive to the test hierarchy which is a dependency we want to avoid. Depending on the class/package hierarchy is ok as refactoring tools will pick up on the reference to the package/class in the rules file.

It also wouldn't help with dealing with the Descriptions created by ErrorReportingRunner. They look something like this:

• package.class
  • package.class#initializationError
  • package.class#initializationError

There is nothing at the top level to differentiate that it's produced by ErrorReportingRunner. If we were relying solely on the Description we have to look inside for an initializationError which is a very fragile mechanism. As it is the best (most robust) way (short of simply making ErrorReportingRunner implement Filterable) is to change every single Filter we have to special case ErrorReportingRunner in the Filter.apply(Object) method.

Request.errorReport(...) has been deprecated and it says that clients should just create ErrorReportingRunner themselves.

[kcooney]

@paulduffin I realize that filtering ErrorReportingRunner is a bit ugly, but 1) it isn't designed to be filtered (it's designed to report critical errors that the test class's runner could not handle) and 2) making it easy to filter ErrorReportingRunner has to be weighted against the real possibility that making itFilterable will break existing users.

Existing filters might not expect to be called with the fake Description objects created by ErrorReportingRunner, so they might throw an exception, or worse, filter out all the children, so the user isn't informed about critical errors.

Can you suggest a better way for your filter to know that the current node is from ErrorReportingRunner? We intentionally try to make the parent node look like a real parent node, so it's reported as an error associated with the test class.

BTW, you are looking at an older version of the code. Request.errorReport() was undeprecated in 4.12. In the current code on master, there is only one case where ErrorReportingRunner will create a Description with more than one child (when you use Request.classes()).

Again, I'd be fine creating an overload of errorReport that took in a class name, but that would only solve your problem if you are calling the method yourself (i.e. you are creating your own runner). If that's the case, then you could just create your own version of ErrorReportingRunner that is filterable.

[kcooney]

One idea: the top-level Description created by ErrorReportingRunner could contain a specific annotation