682798 - support filtering tests by test name as well as test module

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, enhancement, P2)

Description

[Mark Hammond [:markh] [:mhammond]]

It would be great if you could filter by test name inside a test module - eg, to run exactly 1 test function from a module which has multiple test functions.

I see 2 potential strategies:

* Enhance the "-f" option to support, eg "{module_name}:{test_name}" - using a dot might be problematic given '.' has a special meaning to regexes.  Eg, you could say "--filter=test-module:aTestName"

* Introduce a new cmd-line argument specifically for matching the test name - eg, "-filter=test-module --function-filter=aTestName

The first strategy is probably more convenient from the POV of the user (and thus is my preference), but it may have backwards compatibility issues - eg, there is support in unit-test-finder.js for a test *function* as well as a regex.

If either of the above appeals, I'll knock up a patch.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

+1 on the enhancement request generally, and +1 on the first interface strategy specifically (which reminds me of the way I recall Raindrop test filters working).

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: Allow a colon to separate filename filter from testname filter

Attaching a patch using the 'colon' approach.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: updated to include updated --help info

Forgot docs :)  I couldn't find specific docs on this feature so attempted to convey the relevant information in --help output.

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(Pushing all open bugs to the --- milestone for the new triage system)

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Attachment: Allow a colon to separate filename filter from testname filter

Updated for recent changes around this code.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Attachment: Allow a colon to separate filename filter from testname filter

You gotta be quick around here :)  Yet another update for other recent changes.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 560368 [details] [diff] [review]
Allow a colon to separate filename filter from testname filter

+        filterNameRegex = new RegExp(self.filter.substr(colonPos+1));

Nit: conventionally, we surround plus operators (and the like) in JavaScript code with spaces, i.e.:

        filterNameRegex = new RegExp(self.filter.substr(colonPos + 1));


+      filter = function(filename, testname) {
+        if (testname === undefined) {
+          // filtering on just the file name.
+          return filterFileRegex.test(filename);
+        }
+        // To get here we must have already returned 'true' for the filename
+        // itself - so now just filter the test name or return true if no
+        // such filter was supplied.
+        return filterNameRegex ? filterNameRegex.test(testname) : true;
+      };

I'm concerned that this logic depends on behavior that is external to the function.  If that code is later modified to stop calling filterFunction(filename) before filterFunction(filename, testname) at some point, this code will break.

Could we unconditionally check the filename instead?  I.e.:

      filter = function(filename, testname) {
        return filterFileRegex.test(filename) &&
               (testname && filterNameRegex) ? filterNameRegex.test(testname)
                                             : true;
      };

I realize this means testing `filename` unnecessarily when testing each `testname`, but is that really expensive enough to be worth optimizing?

Alternately, and perhaps preferably, we could remove the ability for the TestFinder instantiator to provide a filter function.  As far as I can tell, we don't use that feature in our own code, and I suspect we never intended to provide it, since the original implementation of the TestFinder object didn't have it; it was added as part of a huge E10S-related landing <https://github.com/mozilla/addon-sdk/commit/7ed39b2a99effc09b71209e97ea7825023aff351>.

And my reading of the code suggests it was only added to support the TestFinder constructor setting `this.filter` to a default function if a regex `filter` option isn't provided.  Or, if the feature was intentional, then it seems to have been speculative, a sin second only to premature generalization on the list of evils I intend to banish from the codebase.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Attachment: Updated patch

Thanks for the review and I can't argue with any of your points :)  I agree that dropping the ability to provide a test function makes sense given it is unused and simplifies things - the new patch does this and hopefully addresses all your other points.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 560512 [details] [diff] [review]
Updated patch

Looks great, r=myk!

Comment 10

[Myk Melez [:myk] [@mykmelez]]

https://github.com/mozilla/addon-sdk/commit/ff241ca8b868ce926d4ae8a325331591336656e6