We have a special function in manifestparser called combine_fields. It is used to intelligently combine certain kinds of manifest properties from the DEFAULT section. For instance, if we have:

[DEFAULT]
skip-if = os == "win"

[test_foo]
skip-if = debug

The resulting skip-if value will become skip-if = (os == "win") || (debug). However, when we pass the defaults from a parent manifest down into the child, we do not use combine_fields, and instead the child will simply overwrite the parent's value. E.g:

[include:mochitest.ini]
skip-if = os == "win"

The above skip-if will be overwritten if any of the tests in mochitest.ini have a skip-if key of their own. Instead, these values should be merged.

Through a roundabout way this is blocking some important work. Unfortunately, there are a couple instances of skip-ifs like the above being ignored in the wild. I'll have to follow-up with test owners to see what we should do about them.

Hey Shane, so I discovered this skip-if is being ignored due to this issue:
https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/mochitest.ini#8

I can either A) remove it to keep backwards compatibility once this is fixed, or B) leave it so that it starts working as intended once this is fixed. I assume I should go with B) as the comment suggests it is being skipped to save resources rather than for failures. But just wanted to confirm.

We recently updated the version of pip we use, and it no longer supports
--use-wheels.

Depends on D57404

This was probably a remnant from Mozmill. I don't see any uses of it in mozilla-central
anywhere (or even comm-central for that matter).

Depends on D57405

There aren't any manifests using '[parent:<manifest>]' in mozilla-central.

Depends on D57406

This flag is not passed in from anywhere in mozilla-central.

Depends on D57407

Make it nicer to read and edit.

Depends on D57408

Previously the [DEFAULT] section of a manifest would simply overwrite whatever
values were passed down from the parent. This patch ensures we use
'combine_fields' so things like 'skip-if' and 'support-files' are properly
merged.

Depends on D57409

As of earlier patches in this series, manifestparser correctly combines
defaults from its parent manifests, so the logic the build system used to paper
over this is no longer needed.

I dumped the full test metadata to JSON files both before and after this series. The
diffs were semantically identical.

Depends on D57410

Now that the build system isn't handling its own defaults anymore, we can
remove this flag. Nothing else in mozilla-central uses it.

Depends on D57411

(In reply to Andrew Halberstadt [:ahal] from comment #1)

Hey Shane, so I discovered this skip-if is being ignored due to this issue:
https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/mochitest.ini#8

I can either A) remove it to keep backwards compatibility once this is fixed, or B) leave it so that it starts working as intended once this is fixed. I assume I should go with B) as the comment suggests it is being skipped to save resources rather than for failures. But just wanted to confirm.

Leave it in, but change the comment to "Don't run in-process on windows to save resources". We don't run "in-process" at all for desktop releases, but it's useful to keep testing those on linuxy platforms until mobile supports remote.

Hey Geoff, just wanted to give you a heads up that this might potentially break Thunderbird (assuming you use the copy of manifestparser in mozilla-central). Pinging you since you stepped in the last time I broke you guys, feel free to redirect.

Notably D57407 removes the [parent:<manifest>] tag. I see one instance of this in comm-central, which I think can simply be removed (though not 100% sure).

There are a few other backwards incompatible changes to manifestparser being made.. though I don't see anything in comm-central that should be a problem.

Since we made backwards incompatible changes, let's do a major version bump.

(In reply to Andrew Halberstadt [:ahal] from comment #12)

Notably D57407 removes the [parent:<manifest>] tag. I see one instance of this in comm-central, which I think can simply be removed (though not 100% sure).

Thanks for the heads-up, although I can't see any instances. Can you point me to it?

Also I didn't know such a thing existed, which is a shame because I could've used it several times if I had known. Ah well, it's a bit late now…

(In reply to Geoff Lankow (:darktrojan) from comment #14)

Thanks for the heads-up, although I can't see any instances. Can you point me to it?

https://dxr.mozilla.org/comm-central/source/browser/components/extensions/test/browser/browser.ini#10

Also I didn't know such a thing existed, which is a shame because I could've used it several times if I had known. Ah well, it's a bit late now…

To be clear, you can still have parent manifests. Manifest A can "include" manifest B (thus creating the parent->child relationship). What got removed was the ability for a child to declare what its parent was.. which makes no sense to me. I have no idea what the intended use case of this key was. I suspect it was implemented long ago to solve some weird edge case and hasn't been necessary since.

Oddly, in the link above.. the manifest browser-common.ini is both the child (via the include) and the parent. Don't ask me what that means or how manifestparser is rectifying that internally though :D. I'm somewhat surprised it doesn't cause an infinite loop.

Ah, no wonder I couldn't see it. Clearly the DXR version of comm-central is very out-of-date. That's from a mozilla-central revision 21 months old.

Oh, oops! Looks like searchfox now indexes comm-central (it potentially has been for a long time). I've only been using dxr because I didn't realize this fact yet :). I'll be sure to switch in the future.

https://hg.mozilla.org/mozilla-central/rev/f9003cb89bc6
https://hg.mozilla.org/mozilla-central/rev/f88d7f8b00b5
https://hg.mozilla.org/mozilla-central/rev/f672afcb905e
https://hg.mozilla.org/mozilla-central/rev/ebf5d85517a4
https://hg.mozilla.org/mozilla-central/rev/6671170fb10a
https://hg.mozilla.org/mozilla-central/rev/e46b8e00efce
https://hg.mozilla.org/mozilla-central/rev/c6a0928caab6

ahal: Is this responsible for the following error?

./mach marionette-test toolkit/components/telemetry/tests/marionette/tests/client/manifest.ini

The details of the failure are as follows:

KeyError: 'server_root'

  File "testing/marionette/mach_commands.py", line 109, in marionette_test
    return run_marionette(tests, topsrcdir=self.topsrcdir, **kwargs)
  File "testing/marionette/mach_commands.py", line 58, in run_marionette
    failed = MarionetteHarness(MarionetteTestRunner, args=vars(args)).run()
  File "testing/marionette/harness/marionette_harness/runtests.py", line 71, in run
    runner.run_tests(tests)
  File "testing/marionette/harness/marionette_harness/runner/base.py", line 930, in run_tests
    self.run_test_sets()
  File "testing/marionette/harness/marionette_harness/runner/base.py", line 1135, in run_test_sets
    self.run_test_set(self.tests)
  File "testing/marionette/harness/marionette_harness/runner/base.py", line 1113, in run_test_set
    self.run_test(test['filepath'], test['expected'])
  File "testing/marionette/harness/marionette_harness/runner/base.py", line 1074, in run_test
    **self.test_kwargs)
  File "testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 328, in add_tests_to_suite
    **kwargs))
  File "toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py", line 33, in __init__
    self.testvars["server_root"], self.testvars["server_url"]

self.testvars is {}

Oh, possibly! It certainly seems suspicious.

Though I'm confused about how it's possible because I just grepped the tree and the key server-root doesn't appear in any .ini files in the tree (other than the manifestparser tests). Though, if this was caused by my change how come it didn't get backed out? Does it only happen locally?

If it is my regression and it's urgent to fix, we can back out:
https://hg.mozilla.org/mozilla-central/rev/f672afcb905e

But if it's not as urgent, I'd like to fix this in the telemetry_harness instead of backing out. The ini parser is definitely not the right place to be doing that kind of normalization.

Looking at where that value comes from, I don't think it's related to my patches. Though I haven't bisected to be sure. You can test it on the parent of the revision linked above if you'd like to be sure.

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

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