1604360 - [manifestparser] Keys from parent manifests aren't combined properly

Status: RESOLVED FIXED

(Testing :: Mozbase, defect, P1)

Description

[Andrew Halberstadt [:ahal]]

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.

Comment 1

[Andrew Halberstadt [:ahal]]

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.

Comment 2

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1604360 - [python] Fix ./mach python --ipython, r?mars

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

Comment 3

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1604360 - Fix duplicate keys in the DEFAULT section of some manifests

Depends on D57404

Comment 4

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1604360 - [manifestparser] Remove unused treatment of 'server-root' key

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

Comment 5

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1604360 - [manifestparser] Remove unused 'parent' feature

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

Depends on D57406

Comment 6

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1604360 - [manifestparser] Remove unused 'defaults_only' logic

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

Depends on D57407

Comment 7

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1604360 - [manifestparser] Convert 'test_read_ini' to the pytest format

Make it nicer to read and edit.

Depends on D57408

Comment 8

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1604360 - [manifestparser] Properly merge [DEFAULT] section of manifest with parent defaults

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

Comment 9

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1604360 - [mozbuild] Stop writing/using test-defaults.pkl in the TestManifestBackend, r?#firefox-build-system-reviewers

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

Comment 10

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1604360 - [manifestparser] Remove unused 'handle_defaults=False'

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

Comment 11

[Shane Caraveo (:mixedpuppy)]

(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.

Comment 12

[Andrew Halberstadt [:ahal]]

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.

Comment 13

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1604360 - [manifestparser] Version bump to 2.0.0, r?egao

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

Comment 14

[Geoff Lankow (:darktrojan)]

(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…

Comment 15

[Andrew Halberstadt [:ahal]]

(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.

Comment 16

[Geoff Lankow (:darktrojan)]

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.

Comment 17

[Andrew Halberstadt [:ahal]]

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.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9003cb89bc6
[python] Fix ./mach python --ipython, r=mars
https://hg.mozilla.org/integration/autoland/rev/f88d7f8b00b5
Fix duplicate keys in the DEFAULT section of some manifests r=padenot,mak
https://hg.mozilla.org/integration/autoland/rev/f672afcb905e
[manifestparser] Remove unused treatment of 'server-root' key r=egao
https://hg.mozilla.org/integration/autoland/rev/ebf5d85517a4
[manifestparser] Remove unused 'parent' feature r=egao
https://hg.mozilla.org/integration/autoland/rev/6671170fb10a
[manifestparser] Remove unused 'defaults_only' logic r=egao
https://hg.mozilla.org/integration/autoland/rev/e46b8e00efce
[manifestparser] Convert 'test_read_ini' to the pytest format r=egao
https://hg.mozilla.org/integration/autoland/rev/c6a0928caab6
[manifestparser] Properly merge [DEFAULT] section of manifest with parent defaults r=gbrown

Comment 19

[Andrei Ciure[:aciure]]

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

Comment 20

[Bob Clary [:bc] (inactive)]

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 {}

Comment 21

[Andrew Halberstadt [:ahal]]

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?

Comment 22

[Andrew Halberstadt [:ahal]]

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.

Comment 23

[Andrew Halberstadt [:ahal]]

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.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb77bb2231f3
[manifestparser] Version bump to 2.0.0, r=egao

Comment 25

[Cristian Brindusan [:cbrindusan]]

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

Comment 26

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1604360 - [manifestparser] Remove mention of 'server-root' from the docs, r?egao

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b99373bf1474
[manifestparser] Remove mention of 'server-root' from the docs, r=egao

Comment 28

[Noemi Erli[:noemi_erli]]

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