Open Bug 1344267 Opened 8 years ago Updated 2 years ago

Marionette test files for chrome scope should not be shipped with Firefox

Categories

(Testing :: Marionette Client and Harness, defect, P3)

Version 3
defect

Tracking

(Not tracked)

People

(Reporter: florian, Unassigned)

References

Details

The files packaged at http://searchfox.org/mozilla-central/rev/546a05fec017cb785fca62a5199d42812a6a1fd3/testing/marionette/jar.mn#36 are shipped in Firefox. I don't think that's intended. The 3 following files are reported by my test at bug 1316187 as unreferenced: chrome://marionette/content/test_anonymous_content.xul chrome://marionette/content/test_dialog.properties chrome://marionette/content/test_dialog.xul test_dialog.properties has been recently added in bug 1331037. Are these files chrome mochitests that could be packaged like the examples in http://searchfox.org/mozilla-central/source/toolkit/content/tests/chrome/chrome.ini ?
I’m surprised that ENABLE_TESTS is turned on for release builds. Does this mean we ship test-only binaries such as dist/bin/certutil with officially branded builds? > Are these files chrome mochitests that could be packaged like the > examples in > http://searchfox.org/mozilla-central/source/toolkit/content/tests/chrome/chrome.ini? It’s not totally clear to me what the unique characteristics about chrome resources are and where they need to be to be allowed to run, so my immediate answer is “maybe”. Marionette uses these files to test its chrome interaction abilities. It’s unclear to me if we can load in a file such as test_dialog.xul through navigating to it and expecting the existing tests to work. whimboo may have greater insight in this area.
Flags: needinfo?(florian)
Flags: needinfo?(hskupin)
(In reply to Andreas Tolfsen ‹:ato› from comment #1) > I’m surprised that ENABLE_TESTS is turned on for release builds. ENABLE_TESTS means you want to be able to run tests after building. It's absolutely required for release builds to pass tests before we consider shipping them, so this will be true for most builds you'll encounter. > Does > this mean we ship test-only binaries such as dist/bin/certutil with > officially branded builds? No, random files have to be listed in browser/installer/package-manifest.in to be shipped. (by 'random files' here I mean files that aren't packaged as chrome files).
Flags: needinfo?(florian)
We should very likely work towards a solution that removes these files from testing/marionette/jar.mn. Unsetting ENABLE_TESTS also has other consequences, such as that `./mach marionette-test` will fail when there really is no reason for them to.
(In reply to Florian Quèze [:florian] [:flo] from comment #0) > The 3 following files are reported by my test at bug 1316187 as unreferenced: > chrome://marionette/content/test_anonymous_content.xul > chrome://marionette/content/test_dialog.properties > chrome://marionette/content/test_dialog.xul > > test_dialog.properties has been recently added in bug 1331037. > > Are these files chrome mochitests that could be packaged like the examples > in > http://searchfox.org/mozilla-central/source/toolkit/content/tests/chrome/ > chrome.ini ? Those test files are indeed used to test the chrome handling of Marionette. But there are also others which you are not mentioning here: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/chrome When I eg. search for test_nested_iframe.xul I do not find any reference of that file beside the entry in jar.mn under testing/marionette. So why does your script not list those? Something else we could do for getting chrome tested, is to package all those chrome test files into an add-on which the marionette unit tests will install. This solution would also work if ENABLE_TESTS is set to false.
Flags: needinfo?(hskupin) → needinfo?(florian)
(In reply to Henrik Skupin (:whimboo) from comment #4) > But there are also others which you are not mentioning here: > > https://dxr.mozilla.org/mozilla-central/source/testing/marionette/chrome > > When I eg. search for test_nested_iframe.xul I do not find any reference of > that file beside the entry in jar.mn under testing/marionette. So why does > your script not list those? There's a reference in test.xul (which is unreferenced) : http://searchfox.org/mozilla-central/rev/e844f7b79d6c14f45478dc9ea1d7f7f82f94fba6/testing/marionette/chrome/test.xul#21 My script doesn't use a reference graph yet, so files referenced only from unreferenced flags aren't reported.
Flags: needinfo?(florian)
> Something else we could do for getting chrome tested, is to package > all those chrome test files into an add-on which the marionette unit > tests will install. This solution would also work if ENABLE_TESTS is > set to false. I like that idea. That is in line with how we bootstrap addon dependencies for other harnesses such as mochitest using Marionette.
(In reply to Florian Quèze [:florian] [:flo] from comment #5) > My script doesn't use a reference graph yet, so files referenced only from > unreferenced flags aren't reported. So I'm unclear which immediate action we should take here. What are you proposing? Just adding the .xul files under `support-files` in our unit-test manifest? (In reply to Andreas Tolfsen ‹:ato› from comment #6) > > Something else we could do for getting chrome tested, is to package > > all those chrome test files into an add-on which the marionette unit > > tests will install. This solution would also work if ENABLE_TESTS is > > set to false. > > I like that idea. That is in line with how we bootstrap addon > dependencies for other harnesses such as mochitest using Marionette. While this might sound good, I'm unclear in how long we will still be able to have support for old-style add-ons with XUL pages contained. I think the cut-off date is close. Somewhat in the next versions of Firefox.
Flags: needinfo?(florian)
(In reply to Henrik Skupin (:whimboo) from comment #7) > (In reply to Florian Quèze [:florian] [:flo] from comment #5) > > My script doesn't use a reference graph yet, so files referenced only from > > unreferenced flags aren't reported. > > So I'm unclear which immediate action we should take here. Any solution that doesn't cause test-only files to be shipped to our whole user base is fine with me. I don't have a specific proposal, you know this code better than I do.
Flags: needinfo?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #8) > Any solution that doesn't cause test-only files to be shipped to our whole > user base is fine with me. > I don't have a specific proposal, you know this code better than I do. How do you know that? I might know better our tests, but I have no idea how to load XUL files in chrome scope if we aren't shipping them with Firefox. As far as I know we do not support remote XUL pages anymore, and legacy extensions are no longer supported in Firefox 57. So how can I ship XUL files that they are accessible via the chrome:// scheme? Or are there other options in opening new chrome windows with a XUL file not shipped via chrome://?
Flags: needinfo?(florian)
(In reply to Henrik Skupin (:whimboo) from comment #7) > What are you > proposing? Just adding the .xul files under `support-files` in our unit-test > manifest? You can try this. If it works that's nicely simple. I don't know anything about how the test_*.py tests work, so I don't know if it's likely to work or not. If you end up needing something more complicated, I would look at how mochitest's test harness files are packaged. Eg. stuff at http://searchfox.org/mozilla-central/source/testing/mochitest/jar.mn#34 that seems to be packaged in an add-on by http://searchfox.org/mozilla-central/source/testing/mochitest/moz.build
Flags: needinfo?(florian)
Priority: -- → P5
Priority: P5 → P3
Summary: Marionette test files should not be shipped with Firefox → Marionette test files for chrome scope should not be shipped with Firefox
Severity: normal → S3
Product: Testing → Remote Protocol
Component: Marionette → Marionette Client and Harness
Product: Remote Protocol → Testing
You need to log in before you can comment on or make changes to this bug.