1769257 - run a subset of mochitests with a conditioned profile

Status: RESOLVED FIXED

(Testing :: General, task)

Description

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

n looking at specific tests on mochitest and xpcshell, there are many failures. This bug is about mochitest failures (xpcshell failures are documented in bug 1769154). I suspect most of these are because the profile has more data than what the test case input in.

dom/base/test/ (:hsinyi)

• test_bug564863.xhtml
• test_bug590870.html
• test_title.html

dom/events/test/ (:hsinyi)

• test_bug547996-2.xhtml

dom/tests/mochitest/bugs/ (:hsinyi)

• test_bug369306.html
• test_bug396843.html

dom/tests/mochitest/geolocation/ (:hsinyi)

• test_hidden.html
• test_manyCurrentConcurrent.html
• test_manyCurrentSerial.html
• test_manyWatchConcurrent.html

dom/serviceworkers/test/ (:jstutte)

• test_https_fetch.html
• test_https_origin_after_redirect_cached.html

toolkit/components/extensions/test/mochitest/ (:mixedpuppy)

• test_ext_browsingData_serviceWorkers.html
• test_ext_contentscript_about_blank.html
• test_ext_cookies.html
• test_ext_new_tab_processType.html
• test_ext_scripting_executeScript.html
• test_ext_scripting_removeCSS.html

toolkit/components/url-classifier/tests/mochitest/ (:dimi)

• test_classifier.html

this is a new set of tests, so anything we skip we are not losing coverage, but I would like to know if anything should be split off as a separate bug or ideally not skip-if = condprof when we run the tests?

here is a try push with the failing tests:
https://treeherder.mozilla.org/jobs?repo=try&revision=b6c40be2b2f86f0f98b1a468f390de73b0303cb0&searchStr=mochitest

Comment 1

[Jens Stutte [:jstutte]]

Hi Eden, can you please take a look at the serviceworker ones?

Comment 2

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Hi Andrew, can you please take a look at the DOM-related ones? Thank you.
dom/base/test/

test_bug564863.xhtml
test_bug590870.html
test_title.html

dom/events/test/

test_bug547996-2.xhtml

dom/tests/mochitest/bugs/

test_bug369306.html
test_bug396843.html

dom/tests/mochitest/geolocation/

test_hidden.html
test_manyCurrentConcurrent.html
test_manyCurrentSerial.html
test_manyWatchConcurrent.html

Comment 3

[Andrew McCreight [:mccr8]]

How do I run these tests locally?

Comment 4

[Andrew McCreight [:mccr8]]

A couple of these DOM failures are XHTML files where it says nothing was actually run. Very odd.

Comment 5

[Andrew McCreight [:mccr8]]

I suspect the XHTML failures are because we have allowXULXBL set by default in test profiles, and whatever method is generating these profiles doesn't do the same thing. That said, it is a bit bizarre to have that set, so arguably we should flip the default and make XUL opt-in instead of opt-out. But that would still require some changes to this profile.

Comment 6

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

I have patches in review to make this happen, but for now you would need to:

1. get tip of central
2. apply: https://hg.mozilla.org/try/rev/dfd712ccdf69dc9a9a134fb13823abe77b105c05
3. apply: https://hg.mozilla.org/try/rev/33fe96dce62324c9d5c85882373db43936d0249a

then you should be able to run with ./mach test <path> --conditioned-profile

As for the profile we have a base profile that is updated every day via:
https://treeherder.mozilla.org/jobs?repo=mozilla-central&searchStr=condprof%2Cfull&revision=fe9a9b667b38146755750aeb58cd424491360235

this is downloaded and then we apply our testing prefs on top of that. Quite likely there are conflicting prefs that cause some behavior differences, but I found that most tests run OK and a set of tests have issues (hence this bug)

Comment 7

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

submitted too soon- we do more than prefs, for example the keys4.db file has a conflict and we have to adjust some other files as well. We do use mozprofile to merge the incoming base profile and the testing desired profile.

Comment 8

[Andrew McCreight [:mccr8]]

These tests all fail locally when I change this line to permissions = {}, so presumably that's the issue with the conditioned profiles.
dom/base/test/test_bug564863.xhtml
dom/base/test/test_bug590870.html
dom/base/test/test_title.html
dom/events/test/test_bug547996-2.xhtml
dom/tests/mochitest/bugs/test_bug396843.html

That permission is precisely the sort of test oddness that it sounds like these conditioned profiles is trying to eliminate so in the long term it would be good to not have that, but it could be patched up to get these tests enabled faster if desired.

Still need to figure out why test_bug369306.html and the geolocation tests are failing.

Comment 9

[Kris Maglione [:kmag]]

(In reply to Andrew McCreight [:mccr8] from comment #8)

That permission is precisely the sort of test oddness that it sounds like these conditioned profiles is trying to eliminate so in the long term it would be good to not have that, but it could be patched up to get these tests enabled faster if desired.

I think my preference would be to disable the XULXBL permission by default and have tests that need it opt in. SpecialPowers already has a pushPermissions helper for this sort of thing, but we could also add an option to the mochitest manifest.

Comment 10

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

manifest or test level is fine. I think manifest would require some changes to manifestparser, if we think other harnesses will benefit from this in the future or if we think the total number of tests that would use this is >50 then I would vote for manifest;

I assume we could remove the XULXBL permissions, push to try- see what fails, then look at the total remaining tests and determine if we do manifest or test level. Recently some work was done to add args to the manifestparser (see bug 1762647) that might be a good example of how to do this if we go that route.

Comment 11

[Dimi Lee [:dimi]]

(In reply to Joel Maher ( :jmaher ) (UTC -0800) from comment #0)

toolkit/components/url-classifier/tests/mochitest/ (:dimi)

• test_classifier.html

Filed Bug 1769825 for fixing this testcase failure.

Comment 12

[Kris Maglione [:kmag]]

(In reply to Joel Maher ( :jmaher ) (UTC -0800) from comment #10)

manifest or test level is fine. I think manifest would require some changes to manifestparser, if we think other harnesses will benefit from this in the future or if we think the total number of tests that would use this is >50 then I would vote for manifest;

The only reason I suggested the manifest option is that some of the tests Andrew mentioned have XUL elements embedded in the first-level test file, and I don't think they'll work if we try to set it after they start loading. We'd probably have to move them to second-level files, which is something a lot of legacy tests already do, but which I generally try to avoid.

I think we might be able to just add a generic permissions manifest entry which might be more generally useful, though.

Comment 13

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

Attachment: Bug 1769257 - run a subset of mochitests with a conditioned profile. r=ahal!

Comment 14

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

new tests:
/dom/cache/test/mochitest (:jstutte):

test_cache_padding.html

/dom/system/tests/mochitest (:cmartin):

test_bug1197901.html

2 existing ones + 1 new one
dom/serviceworkers/test/ (:echuang)

    test_https_fetch.html
    test_https_origin_after_redirect_cached.html
  +test_openwindow.html

existing tests here, but 2 have been removed!
toolkit/components/extensions/test/mochitest/ (:mixedpuppy)

    test_ext_browsingData_serviceWorkers.html
    test_ext_contentscript_about_blank.html
    test_ext_cookies.html
    test_ext_new_tab_processType.html
   - test_ext_scripting_executeScript.html
   - test_ext_scripting_removeCSS.html

the patch up for review will skip 14 tests total including some of these new tests. As a note, we are not turning off tests, just skipping on a new test variant.

:jstutte can you determine if the cache test needs attention when run in a conditioned profile, or ok to skip?
:cmartin can you determine if the dom/system test needs attention when run in a conditioned profile, or ok to skip?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e22a2eaac044
run a subset of mochitests with a conditioned profile. r=ahal

Comment 16

[Narcis Beleuzu [:NarcisB]]

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

Comment 17

[Jens Stutte [:jstutte]]

Hi :jmaher, sorry for coming late here. Is my input still needed here (given that it's closed)?

Comment 18

[Andrew McCreight [:mccr8]]

(In reply to Jens Stutte [:jstutte] from comment #17)

Hi :jmaher, sorry for coming late here. Is my input still needed here (given that it's closed)?

The tests still need to be fixed. This patch was able to land because jmaher disabled the failing tests.

Comment 19

[Jens Stutte [:jstutte]]

Gotcha. Moving work then to the dedicated bugs just created.

Comment 20

[Chris Martin [:cmartin]]

Created a bug for me to investigate and re-enable the sensor test