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

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Jonathan Griffin (:jgriffin)]

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.

Comment 1

[Jonathan Griffin (:jgriffin)]

Attachment: patch v0.1

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.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

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)

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 4

[Jonathan Griffin (:jgriffin)]

I removed the extra whitespace.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cefb4ae9e7d5

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cefb4ae9e7d5