1035512 - (sdk-mochitestification-fx) Mochitestification for Firefox

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P4)

Comment 1

[Dave Townsend [:mossop]]

Attachment: Add-on SDK changes

First of what may be a few roll-up patches to make the SDK work with the mochitest harness. Most of this is because of switching from an add-on to bare files and from cuddlefish to the bare loader.

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8454146 [details] [review]
Add-on SDK changes

Some nits

Comment 3

[Dave Townsend [:mossop]]

Attachment: final SDK pieces

These are the final bits to change in the SDK, mostly to allow test harness re-use and using the same runtime for all add-on tests.

Comment 4

[Dave Townsend [:mossop]]

Attachment: Build system changes

This adds support for two new manifest types, mostly mirroring what browser-chrome does, for jetpack-package tests (common JS modules) and jetpack-addon tests (built add-ons). The latter are built as part of the build process and so some of the checks for missing tests are skipped.

Comment 5

[Dave Townsend [:mossop]]

Attachment: Mochitest harness changes

This is the minor changes to the mochitest harness and mach commands. Just adding support for two new test types, jetpack-package and jetpack-addon. In both cases the harness just overlays some chrome XUL onto the main window after running Firefox. These will be attached in the next patch as it probably makes sense for the in-Firefox JS to be reviewed by someone else.

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8473965 [details] [diff] [review]
Build system changes

Review of attachment 8473965 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice patch!

I'd like to run this by someone in #ateam for final sign-off of the separate manifest types. We've recently added "subsuites" and one can make the argument that jetpack tests are a subsuite of mochitests. They may have plans here and I don't want to undercut them.

::: addon-sdk/Makefile.in
@@ +30,5 @@
> +				base=`basename $$dir` ; \
> +				($(srcdir)/source/bin/cfx xpi --pkgdir=$$dir --output-file=$(TESTROOT)/$$base.xpi *) \
> +			fi \
> +		done \
> +	fi

Yikes.

What's the run-time of this?

If more than a second or two, we should rewrite this to evaluate as multiple rules. Having a single loop like this for steps that can execute concurrently only makes the build slower. We should be able to leverage $(wildcard), $(eval), and $(call) to do this.

Also, this reminds me that we really need a better way to define XPIs in the build system. Hopefully glandium's work around templates can provide something useful... But let's not scope creep.

Comment 7

[Dave Townsend [:mossop]]

Attachment: JS test harnesses

These are the JS parts of the harness loaded into the browser by runtests.py. Feel up to reviewing these Erik? Each is loaded depending on the type of test and just get the list of tests then run them all and report back the results.

Comment 8

[Dave Townsend [:mossop]]

The plan here will be to land these patches after review then talk to releng about getting the tests running probably on a test branch to work out any final kinks in tinderbox.

Comment 9

[Dave Townsend [:mossop]]

Attachment: Build system changes rev 2

This looks like it's probably better. It's certainly building all the XPIs in parallel. I can be sure because I found two race conditions in the cfx tool! Creating the same directory for every XPI seems inefficient too, there is probably a better place to put that?

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8474019 [details] [diff] [review]
Build system changes rev 2

Review of attachment 8474019 [details] [diff] [review]:
-----------------------------------------------------------------

::: addon-sdk/Makefile.in
@@ +15,5 @@
>  	$(MAKE) -f copy_source.mk libs
>  
> +TESTADDONS = source/test/addons
> +ADDONSRC = $(srcdir)/$(TESTADDONS)
> +TESTROOT = $(CURDIR)/$(DEPTH)/_tests/testing/mochitest/jetpack-addon/$(relativesrcdir)/$(TESTADDONS)

You shouldn't need $(CURDIR) here.

@@ +21,5 @@
> +# Build a list of the test add-ons
> +ADDONS = $(patsubst $(ADDONSRC)/%/package.json,%.xpi,$(wildcard $(ADDONSRC)/*/package.json))
> +
> +# This can switch to just zipping the files when native jetpacks land
> +%.xpi:

You should put a path prefix on this wildcard rule so it doesn't exactly match other rules elsewhere in the build system. (I don't think we have any, but this is a general make style thing.)

@@ +23,5 @@
> +
> +# This can switch to just zipping the files when native jetpacks land
> +%.xpi:
> +	$(NSINSTALL) -D $(TESTADDONS)
> +	$(srcdir)/source/bin/cfx xpi --pkgdir=$(ADDONSRC)/$(patsubst %.xpi,%,$@) --output-file=$(TESTADDONS)/$@

So, a problem with this approach is that there are no dependencies. If the input into the .xpi changes, the xpi won't get rebuilt. That would mean clobber builds. And that's a worse evil than slower builds.

I don't suppose cfx can output a make dependencies file, can it? If not, you'll want to chain a dummy target as a dependency to force building. The "FORCE" target can do that.

Comment 11

[Dave Townsend [:mossop]]

(In reply to Gregory Szorc [:gps] from comment #10)
> @@ +23,5 @@
> > +
> > +# This can switch to just zipping the files when native jetpacks land
> > +%.xpi:
> > +	$(NSINSTALL) -D $(TESTADDONS)
> > +	$(srcdir)/source/bin/cfx xpi --pkgdir=$(ADDONSRC)/$(patsubst %.xpi,%,$@) --output-file=$(TESTADDONS)/$@
> 
> So, a problem with this approach is that there are no dependencies. If the
> input into the .xpi changes, the xpi won't get rebuilt. That would mean
> clobber builds. And that's a worse evil than slower builds.

So right now it clobbers every time, because the xpi it thinks it is trying to build (addon-sdk/%.xpi) is actually built into (addon-sdk/source/test/addons/%.xpi). The dependencies are every file in the source directory. Is there a straightforward way to say that?

> I don't suppose cfx can output a make dependencies file, can it? If not,
> you'll want to chain a dummy target as a dependency to force building. The
> "FORCE" target can do that.

CFX can't do it, it isn't impossible but it would be wasted work as the plan is to switch away from CFX to plain ZIP in the near future.

Comment 12

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

Comment on attachment 8473968 [details] [diff] [review]
Mochitest harness changes

Review of attachment 8473968 [details] [diff] [review]:
-----------------------------------------------------------------

In general these two new types of harnesses inside of mochitest are not very attractive, we just keep shoving more and more into the harness.  Thinking about what other options we have in our current system, I don't see other viable options.

A few things I would like to know (unrelated to this patch):
1) do you use structured logging in the harness and tests?
2) do you use manifests to define and filter the tests?
3) are there issues with e10s or special powers?
4) we have done a lot of work this year for --run-by-dir and --bisect-chunk, I imagine we would need to port that work to these two new harnesses

Regarding this specific patch, I only have a few minor things that caught my eye the rest looks good.

::: testing/mochitest/runtests.py
@@ +487,5 @@
>  
>      # Note that all tests under options.subsuite need to be browser chrome tests.
>      if options.browserChrome or options.chrome or options.subsuite or \
> +       options.a11y or options.webapprtChrome or options.jetpackPackage or \
> +       options.jetpackAddon:

this is starting to get ugly with 7 or conditions, please file a bug to fix this.  I see us probably creating a overlay flag or a plain flag which we can just check.

@@ +567,5 @@
>        allow_js_css = True
>        testPattern = re.compile(r"browser_.+\.js")
> +    elif options.jetpackPackage:
> +      allow_js_css = True
> +      testPattern = re.compile(r"test-.+\.js")

why are these test-* instead of test_* ?

@@ +797,5 @@
>  
> +    if options.jetpackPackage:
> +      chrome += """
> +overlay chrome://browser/content/browser.xul chrome://mochikit/content/jetpack-package-overlay.xul
> +"""

do you need navigator.xul and shell.xhtml to be overwritten as well?

Comment 13

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

Comment on attachment 8474019 [details] [diff] [review]
Build system changes rev 2

Review of attachment 8474019 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks mossop for making this happen!

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +830,5 @@
>                  filtered = m.active_tests(exists=False, disabled=True,
>                      **self.info)
>  
> +                # Jetpack add-on tests are expected to be generated during the
> +                # build process so they won't exist here.

Is this auto generation of tests done in the Makefile.in?

Comment 14

[Dave Townsend [:mossop]]

(In reply to Joel Maher (:jmaher) from comment #12)
> Comment on attachment 8473968 [details] [diff] [review]
> Mochitest harness changes
> 
> Review of attachment 8473968 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In general these two new types of harnesses inside of mochitest are not very
> attractive, we just keep shoving more and more into the harness.  Thinking
> about what other options we have in our current system, I don't see other
> viable options.
> 
> A few things I would like to know (unrelated to this patch):
> 1) do you use structured logging in the harness and tests?

Not sure what you're asking here. It does log the "TEST-PASS" and "TEST-UNEXPECTED-FAIL" lines if that is what you mean.

> 2) do you use manifests to define and filter the tests?

These patches add then yes.

> 3) are there issues with e10s or special powers?

We have a couple of tests that open an e10s window to run, they're a little flakey right now. We have a GSoC student working to make everything work in e10s. We don't use special powers.

> 4) we have done a lot of work this year for --run-by-dir and --bisect-chunk,
> I imagine we would need to port that work to these two new harnesses

The JS part of the test harness uses the chunkify code so this might be done already but I'd rather get these parts landed before checking and getting all that running. As it is the number of tests is small enough that I doubt we'll need to use chunking for a while.

> Regarding this specific patch, I only have a few minor things that caught my
> eye the rest looks good.
> 
> ::: testing/mochitest/runtests.py
> @@ +487,5 @@
> >  
> >      # Note that all tests under options.subsuite need to be browser chrome tests.
> >      if options.browserChrome or options.chrome or options.subsuite or \
> > +       options.a11y or options.webapprtChrome or options.jetpackPackage or \
> > +       options.jetpackAddon:
> 
> this is starting to get ugly with 7 or conditions, please file a bug to fix
> this.  I see us probably creating a overlay flag or a plain flag which we
> can just check.

Filed bug 1055094

> @@ +567,5 @@
> >        allow_js_css = True
> >        testPattern = re.compile(r"browser_.+\.js")
> > +    elif options.jetpackPackage:
> > +      allow_js_css = True
> > +      testPattern = re.compile(r"test-.+\.js")
> 
> why are these test-* instead of test_* ?

I'm not sure why the jetpack tests used this naming style, the reasons are lost in the mists of time. I don't think there is a strong reason but renaming all the tests now has some cost.

> @@ +797,5 @@
> >  
> > +    if options.jetpackPackage:
> > +      chrome += """
> > +overlay chrome://browser/content/browser.xul chrome://mochikit/content/jetpack-package-overlay.xul
> > +"""
> 
> do you need navigator.xul and shell.xhtml to be overwritten as well?

At this time I've only checked this all works in Firefox. If we find ourselves needing to run in other apps we can make those changes then.

(In reply to Joel Maher (:jmaher) from comment #13)
> Comment on attachment 8474019 [details] [diff] [review]
> Build system changes rev 2
> 
> Review of attachment 8474019 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks mossop for making this happen!
> 
> ::: python/mozbuild/mozbuild/frontend/emitter.py
> @@ +830,5 @@
> >                  filtered = m.active_tests(exists=False, disabled=True,
> >                      **self.info)
> >  
> > +                # Jetpack add-on tests are expected to be generated during the
> > +                # build process so they won't exist here.
> 
> Is this auto generation of tests done in the Makefile.in?

Currently yes. As tests only come from one place right now that seems simplest. I can imagine wanting to make it a more formal part of the build system if we use this test style elsewhere in the tree in the future.

Comment 15

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

thanks for the information.  We will need to file bugs for adding structured logging into here (bug 916295 tracks that) and job bisection (bug 1036372 tracks that).

filing these bugs shouldn't stop us from moving forward.  Likewise the test names are fine as they are, but maybe a follow up bug to have them test_ just to make things uniform.

Comment 16

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/909d37ddce2860e72b2220db538fdfd118a41467
Merge pull request #1590 from Mossop/mochitestification-2

Bug 1035512: SDK pieces for mochitests. r=erikvold.

This allows the test harness to be kept open after tests complete so we can run add-on tests in the same runtime. It also cleans up consoleListener properly if we want to re-use the test harness in a single loader.

Comment 17

[Dave Townsend [:mossop]]

Attachment: Build system changes rev 3

This only adds some changes to Makefile.in, specifically:

* Calling python directory rather than letting the cfx script do it (caused failures on windows)
* Force building the XPIs each time, we can stop doing this once we switch away from cfx and have sane XPI building in tree
* Fix the references for the old TEST_FILES to always pull from the srcdir since it was using the objdir instead with these changes. These old ways of including the tests will go away once we have this new harness up and running fully.

Comment 18

[Dave Townsend [:mossop]]

Comment on attachment 8486829 [details] [diff] [review]
Build system changes rev 3

I guess gps is out. Ted, the interdiff for this patch shows the changes since the last reviews.

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 8486829 [details] [diff] [review]
Build system changes rev 3

Review of attachment 8486829 [details] [diff] [review]:
-----------------------------------------------------------------

::: addon-sdk/Makefile.in
@@ +21,5 @@
> +# Build a list of the test add-ons
> +ADDONS = $(patsubst $(ADDONSRC)/%/package.json,$(TESTADDONS)/%.xpi,$(wildcard $(ADDONSRC)/*/package.json))
> +
> +$(CURDIR)/$(TESTADDONS):
> +	$(NSINSTALL) -D $@

You want to use $(call mkdir_deps) in the dependency list below.

@@ +25,5 @@
> +	$(NSINSTALL) -D $@
> +
> +# This can switch to just zipping the files when native jetpacks land
> +$(TESTADDONS)/%.xpi: FORCE $(CURDIR)/$(TESTADDONS) $(ADDONSRC)/%
> +	$(PYTHON) $(srcdir)/source/bin/cfx xpi --pkgdir=$(lastword $^) --output-file=$@

We really need to figure out how to express this in moz.build so we don't have to do it on every single build.

@@ +28,5 @@
> +$(TESTADDONS)/%.xpi: FORCE $(CURDIR)/$(TESTADDONS) $(ADDONSRC)/%
> +	$(PYTHON) $(srcdir)/source/bin/cfx xpi --pkgdir=$(lastword $^) --output-file=$@
> +
> +libs:: $(ADDONS)
> +	$(INSTALL) $^ $(TESTROOT)

Can you use INSTALL_TARGETS here?

Comment 20

[Dave Townsend [:mossop]]

Landed at last

https://hg.mozilla.org/integration/fx-team/rev/1401f98a790e

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1401f98a790e

Comment 22

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/b858154c55d73e8a15cee6db67f4ea28209da58c
Bug 1035512: Add a new common JS testing harness based on the mochitest test runner. r=gps, ted, erikvold, jmaher

Comment 23

[Erik Vold [:erikvold] (please needinfo? me)]

Pretty sure bug 1036625 is not a blocker here any longer due to the fix in bug 1077078.