1468677 - ./mach jstestbrowser --filter option runs no tests

Status: RESOLVED FIXED

(Testing :: Reftest, enhancement)

Description

[Steve Fink [:sfink] [:s:]]

If you use the --filter option, no tests at all will be run (regardless of what filter you choose.)

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: [reftest] remove dummy reftest.manifests entry used for global filter

The problem is that the filter is passed as the "global filter" by making an entry in the reftest.manifests pref keyed off of the empty string. reftest.jsm then tries to load the empty string as a URL, which fails and aborts the manifest parsing entirely.

The surrounding code is still a little scary -- manifests[""] is the global filter, manifests[null] is the "urlsFilterRegex", whatever that means (it seems unused.) But this seemed to fix something, so I'll just submit it.

Comment 2

[Andrew Halberstadt [:ahal]]

Comment on attachment 8985305 [details] [diff] [review]
[reftest] remove dummy reftest.manifests entry used for global filter

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

This is where the filter is being set:
https://searchfox.org/mozilla-central/source/layout/tools/reftest/runreftest.py#560

I'm not really sure why we're stuffing this in the manifests pref, maybe it would be better if it were a separate pref? Then again maybe there is some sort of reason things are the way they are. So if this fixes your issue (and doesn't cause any others in CI), then r+ from me.

Comment 3

[Steve Fink [:sfink] [:s:]]

(In reply to Andrew Halberstadt [:ahal] from comment #2)
> Comment on attachment 8985305 [details] [diff] [review]
> [reftest] remove dummy reftest.manifests entry used for global filter
> 
> Review of attachment 8985305 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is where the filter is being set:
> https://searchfox.org/mozilla-central/source/layout/tools/reftest/runreftest.
> py#560

Right, sorry, I should have mentioned that that's where I tracked it down from.

> I'm not really sure why we're stuffing this in the manifests pref, maybe it
> would be better if it were a separate pref? Then again maybe there is some
> sort of reason things are the way they are. So if this fixes your issue (and
> doesn't cause any others in CI), then r+ from me.

Yes, it does seem wrong to me, but I was cowering behind the excuse of making a minimal change to stuff I don't understand. The whole thing with mapping manifests to filters seems a little weird to me; I don't understand why you'd have a per-manifest filter to begin with. And added to it the weirdness of manifests[""] vs manifests[null] vs manifests[manifestURL]... well, I'd rather not touch it for fear of breaking something.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bc9dd454c78
[reftest] remove dummy reftest.manifests entry used for global filter, r=ahal

Comment 5

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2bc9dd454c78