948278 - Avoid piggy-backing into the build system for the reftest addon

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Avoid piggy-backing into the build system for the reftest addon

https://tbpl.mozilla.org/?tree=Try&rev=b456833a6280

Comment 2

[(not currently active) Ted Mielczarek]

Historically I wrote things this way because I needed to get the Reftest extension into the test package in a usable way, but dbaron relied on running "firefox -reftest" directly from the objdir. This patch would break that use case, so I have to defer to dbaron as to whether he's still using things that way.

Comment 3

[Gregory Szorc [:gps]]

Why can't people run reftests through the sanctioned harness? We should be trending towards unifying the execution methodology between official automation and local development, hopefully for obvious reasons. If that's not reason enough, the hackiness that is the dual packaging of the http server is unique in the build system and represents a significant hurdle towards making jar processing and packaging more robust and faster. I would very much like to excise this requirement so that the solution is greatly simplified.

Comment 4

[(not currently active) Ted Mielczarek]

I agree, but dbaron is the reftest owner, so he's the one you need to convince here. If he's still relying on this functionality then we'll have to figure out some way to fix the build system that doesn't involve excising that.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Gregory Szorc [:gps] (in southeast Asia until Dec 14) from comment #3)
> Why can't people run reftests through the sanctioned harness? We should be
> trending towards unifying the execution methodology between official
> automation and local development, hopefully for obvious reasons. If that's
> not reason enough, the hackiness that is the dual packaging of the http
> server is unique in the build system and represents a significant hurdle
> towards making jar processing and packaging more robust and faster. I would
> very much like to excise this requirement so that the solution is greatly
> simplified.

Reftest isn't only a tool for running in our harnesses; it's also a tool that can be used in other ways, for example, running reftests that are part of a W3C standards compliance suite.  It's also often nice for debugging to be able to use tools atomically rather than figure out how to use them through multiple layers of automation, which adds to the burden of figuring out how to hack on Gecko.  (It certainly has for me, anyway; going through multiple layers of automation ends up requiring answering fun questions like "how do I set this environment variable when Firefox runs, but not for all the other processes that the automation starts?  Yes, I know there's an answer to that particular question now, but the point is that when you're working with things that the people who developed the automation didn't think about, it's nice to have the costs of using the new thing be lower rather than higher.)


What effect does this patch have, anyway?

Comment 6

[Mike Hommey [:glandium]]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #5)
> What effect does this patch have, anyway?

Essentially, it removes the reftest components from dist/bin/components. The addon is still available, so that it can be installed (although, inconveniently, it's not packaged as an xpi)

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So it's making the build-reftest-as-an-addon mechanism that was added in bug 468913 the only mechanism for using reftest?

Comment 8

[Mike Hommey [:glandium]]

Yes.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 8345133 [details] [diff] [review]
Avoid piggy-backing into the build system for the reftest addon

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

This patch is technically fine, but I don't want to take it unless we satisfy dbaron's concerns.

Comment 10

[Joshua Cranmer [:jcranmer]]

Dupe of bug 1098537?

Comment 12

[Mike Hommey [:glandium]]

Attachment: Avoid piggy-backing into the build system for the reftest addon

This is mostly the same as the old patch, refreshed against current tip, and it also makes the reftest addon use the httpd server as a js module instead of a XPCOM component.

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8525217 [details] [diff] [review]
Avoid piggy-backing into the build system for the reftest addon

David, could check the resulting addon still works for your usecase? (clean up $objdir/dist/xpi-stage/reftest before building)

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So with this patch, if I want to use reftest directly without going through runreftest.py, mach, etc., how would I do that?

Comment 15

[Mike Hommey [:glandium]]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #14)
> So with this patch, if I want to use reftest directly without going through
> runreftest.py, mach, etc., how would I do that?

You install the addon and use firefox -reftest? Isn't that what you're doing?

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I've never had to install an addon in the past; firefox -reftest just works in an --enable-tests build.  That was how it was originally written.

Comment 6 suggest installing the addon might need to be done in some unusual way -- is that the case?

(It's also not clear to me how the issue of using the HTTP server as a JS module vs. an XPCOM component relates to the rest of it.)

Comment 17

[Mike Hommey [:glandium]]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #16)
> I've never had to install an addon in the past; firefox -reftest just works
> in an --enable-tests build.  That was how it was originally written.

I thought one of the reasons for the addon was for you to run reftest on downloaded versions of firefox. Is that not the case?
 
> Comment 6 suggest installing the addon might need to be done in some unusual
> way -- is that the case?

With the patch as it is, yes. But surely using e.g. runreftest.py would make that smoother. BTW, I don't get comment 14. What are your reasons for wanting to use reftest "directly without going through runreftest.py, mach, etc."?

> (It's also not clear to me how the issue of using the HTTP server as a JS
> module vs. an XPCOM component relates to the rest of it.)

That one of the things that makes the build system parts awkward. The previous patch didn't do it, and that still left awkward things in the build system.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Mike Hommey [:glandium] from comment #17)
> I thought one of the reasons for the addon was for you to run reftest on
> downloaded versions of firefox. Is that not the case?

I believe it was part of the setup for doing test packaging and running the tests on a machine separate from the builder.  I don't recall why it was needed, although Ted probably would (see comment 2 and bug 468913).

> > Comment 6 suggest installing the addon might need to be done in some unusual
> > way -- is that the case?
> 
> With the patch as it is, yes. But surely using e.g. runreftest.py would make
> that smoother. BTW, I don't get comment 14. What are your reasons for
> wanting to use reftest "directly without going through runreftest.py, mach,
> etc."?

Two reasons:

 (1) having to deal with lots of layers makes it harder to do things that the developers of those layers didn't anticipate or plan for:

    (a) One simple example is running using various debugging tools (whether programs like gdb rr or valgrind, command line options to the browser, environment variables for the browser execution, or preferences to set in the profile -- very basic things that I think still aren't uniformly supported across mach testing targets).  Test suites need to be written not only to be run, but to be debugged!  And debugging often requires different tools for different problems.  (A more advanced example -- though one I haven't actually needed for quite a while -- is running the tests on an existing profile rather than creating a fresh one.)  These layers all tend to do different things which produces inconsistencies, such as the current working directory when running different test suites being different between suites, leading to the "hunt around for the log file" problem.

    (b) Another example is running tests on machines other than the build machines (e.g., when needing to run tests on a machine with a different graphics driver) -- with reftest it's relatively easy to rsync some files over to another machine (but not the full build tree) and run reftests; with mochitests, which rely on the layers of python much more heavily, it's quite complicated (though I've done it, and over a relatively-constrained network connection to boot).  (Having the build system have a target for generating packaged tests would have helped with that one.)

    (c) Another is using the test harnesses to run things other than our tests, e.g., using the reftest harness to run tests in a W3C test suite (which I've done to generate results for the CSS 2.1 test suite).

 (2) the more layers there are, the harder it is, and the less likely people are, to fix a problem that requires passing information through all of the layers.


That said, if you need to do this to make progress on the build system, you should just do it.  But I'd appreciate having the reftest documentation describe how to run reftest without runreftests.py or mach so that people know how to do it if they need to.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8525217 [details] [diff] [review]
Avoid piggy-backing into the build system for the reftest addon

see previous comment

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 8525217 [details] [diff] [review]
Avoid piggy-backing into the build system for the reftest addon

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

If dbaron is okay with the state of affairs here then have at it.

::: layout/tools/reftest/Makefile.in
@@ +58,2 @@
>  	$(INSTALL) $(_HARNESS_FILES) $(_DEST_DIR)
>  	(cd $(DIST)/xpi-stage && tar $(TAR_CREATE_FLAGS) - reftest) | (cd $(_DEST_DIR) && tar -xf -)

Do you have a plan for tearing this out?

Comment 21

[Gregory Szorc [:gps]]

froydnj was looking at that in the bug that did TEST_HARNESS_FILES. Someone (me?) convinced him to reduce scope and tackle the problem later. Now might be a good time for that :)

Comment 22

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78a5cadec4fc

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/78a5cadec4fc