Florian Quèze [:florian] Reporter
	Description • 7 years ago

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 ?

	Andreas Tolfsen ❲:ato❳
	Comment 1 • 7 years ago

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.

	Florian Quèze [:florian] Reporter
	Comment 2 • 7 years ago

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

	Andreas Tolfsen ❲:ato❳
	Comment 3 • 7 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 4 • 7 years ago

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

	Florian Quèze [:florian] Reporter
	Comment 5 • 7 years ago

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

	Andreas Tolfsen ❲:ato❳
	Comment 6 • 7 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 7 • 7 years ago

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

	Florian Quèze [:florian] Reporter
	Comment 8 • 7 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 9 • 7 years ago

(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://?

	Florian Quèze [:florian] Reporter
	Comment 10 • 7 years ago

(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