support filtering tests by test name as well as test module

RESOLVED FIXED

Status

Add-on SDK
General
P2
enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 years ago
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.
+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).
(Assignee)

Comment 2

6 years ago
Created attachment 557070 [details]
Allow a colon to separate filename filter from testname filter

Attaching a patch using the 'colon' approach.
(Assignee)

Comment 3

6 years ago
Created attachment 557075 [details] [diff] [review]
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.
Attachment #557070 - Attachment is obsolete: true
Assignee: nobody → mhammond
Priority: -- → P2
Target Milestone: --- → 1.2
(Pushing all open bugs to the --- milestone for the new triage system)
Target Milestone: 1.2 → ---
(Assignee)

Comment 5

6 years ago
Created attachment 560360 [details] [diff] [review]
Allow a colon to separate filename filter from testname filter

Updated for recent changes around this code.
Attachment #557075 - Attachment is obsolete: true
Attachment #560360 - Flags: review?(myk)
(Assignee)

Comment 6

6 years ago
Created attachment 560368 [details] [diff] [review]
Allow a colon to separate filename filter from testname filter

You gotta be quick around here :)  Yet another update for other recent changes.
Attachment #560360 - Attachment is obsolete: true
Attachment #560360 - Flags: review?(myk)
Attachment #560368 - Flags: review?(myk)
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.
(Assignee)

Comment 8

6 years ago
Created attachment 560512 [details] [diff] [review]
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.
Attachment #560368 - Attachment is obsolete: true
Attachment #560368 - Flags: review?(myk)
Attachment #560512 - Flags: review?(myk)
Comment on attachment 560512 [details] [diff] [review]
Updated patch

Looks great, r=myk!
Attachment #560512 - Flags: review?(myk) → review+
https://github.com/mozilla/addon-sdk/commit/ff241ca8b868ce926d4ae8a325331591336656e6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.