Closed Bug 781711 Opened 9 years ago Closed 9 years ago

Mochitest manifests don't work properly when both runtests and excludetests are present

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla17

People

(Reporter: jgriffin, Assigned: jgriffin)

Details

Attachments

(1 file)

For example, suppose you have a manifest like so:

{ "runtests": {
  "dom/indexedDB": ""
  },
  "excludetests": {
  "dom/indexedDB/test/test_create_index.html"
  }
}

The intent is to run everything inside dom/indexedDB *except* test_create_index.html.

Instead, everything gets runs.

This happens because the logic which filters based on runtests and excludetests first excludes all excludetests, then adds everything that matches runtests.  It should work the other way around, when both are specified.
Attached patch patch v0.1Splinter Review
This works both in the case I described in the description, as well as the 'android.json' case in which tests are only excluded.

This also fixes the CLI so you can pass --test-manifest without generating an error.
Attachment #650748 - Flags: review?(jmaher)
Comment on attachment 650748 [details] [diff] [review]
patch v0.1

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

logic makes sense and code looks good.  I get the feeling that we are missing something simple, maybe because this code is sort of hairy (it was hacky to begin with, now it is morphed a bit with some cleanup and more hacks)
Attachment #650748 - Flags: review?(jmaher) → review+
Comment on attachment 650748 [details] [diff] [review]
patch v0.1

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

::: testing/mochitest/tests/SimpleTest/setup.js
@@ +235,5 @@
> +      for (var f in runtests) {
> +        // Remove leading /tests/ if exists
> +        file = f.replace(/^\//, '')
> +        file = file.replace(/^tests\//, '')
> +        

Trailing whitespace

@@ +260,5 @@
> +      for (var f in excludetests) {
> +        // Remove leading /tests/ if exists
> +        file = f.replace(/^\//, '')
> +        file = file.replace(/^tests\//, '')
> +        

And here
I removed the extra whitespace.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cefb4ae9e7d5
Assignee: nobody → jgriffin
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/cefb4ae9e7d5
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.