1693063 - Composite skip-if pattern is displayed incorrectly

Status: NEW

(Testing :: General, defect, P3)

Description

[Simon Giesecke [:sg] [he/him]]

When I go to https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_storage_idb_data_migration.js, the skip pattern is shown as os == "android" appname == "thunderbird" || (os == "android" && debug).

This somehow as if some delimiter is missing when parsing multiple skip entries from the ini file. Probably this should be something like os == "android" || appname == "thunderbird" || (os == "android" && debug) (though this indicates that the skip patterns are somewhat redundant).

Comment 1

[Andrew McCreight [:mccr8]]

The main ini file has this entry:

skip-if = appname == "thunderbird" || (os == "android" && debug)

That ini file is included via this line, which is presumably where the first Android clause is coming from:

[include:xpcshell-common.ini]
run-if = os == 'android' # Android has no remote extensions, Bug 1535365

However, it is also included via another ini file which does NOT have the Android exclusion, so I think the initial Android clause should not be included.

Comment 2

[Andrew McCreight [:mccr8]]

A fully correct implementation probably would require some kind of binary decision diagram to canonicalize the logical statements. ;)

Comment 3

[Simon Giesecke [:sg] [he/him]]

Ah, I wasn't aware of run-if at all. I tried to find a skip-if = os == 'android' entry somewhere, but obviously couldn't find it.

Comment 4

[Andrew McCreight [:mccr8]]

Oh, right, I didn't even notice that the include had a run-if instead of a skip-if. The xpcshell-remote.ini file has a skip-if = os == "android" at the top so maybe that is where the os == "android" is coming from?

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

This is what's in test-info-all-tests.json:

      {
        "skip-if": "os == \"android\"\nappname == \"thunderbird\" || (os == \"android\" && debug)",
        "test": "toolkit/components/extensions/test/xpcshell/test_ext_storage_idb_data_migration.js"
      },

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

That data comes from the mach "test-info report" command as run by the test-info-all job:

./mach test-info report --show-tests --show-summary --show-activedata --verbose --output-file /builds/worker/artifacts/test-info-all-tests.json

Comment 7

[Andrew McCreight [:mccr8]]

Thanks. It sounds like this is a bug in the data that SearchFox is ingesting then.

Comment 8

[Andrew McCreight [:mccr8]]

browser/components/sessionstore/test/browser_history_persist.js is showing a similar error. It says "This test gets skipped with pattern: os == 'linux' && !e10s fission", which looks like a combination of "skip-if = os == 'linux' && !e10s" from the beginning of the .ini file and "skip-if = fission" from the specific test. I'll revert the summary because it doesn't seem to require include, just the combination of skip patterns.

Comment 9

[Andrew Halberstadt [:ahal]]

Yeah, the format of skip-if is actually a list of newlines implicitly joined by ||. So in comment 8 the test has a skip-if that gets joined with the one in the DEFAULT section. Here's one that has two distinct skip lines directly:
https://searchfox.org/mozilla-central/rev/2cc3b39bc9bb024d35e09e9c8acecf0e2dfb4e13/browser/components/sessionstore/test/browser.ini#286

(it also exhibits this error)

So I guess mach test-info (or whatever is generating this data) needs to take this into account. Ideally it should be using manifestparser to parse the manifests, but I guess it's not.