351968 - ENABLE_TESTS has side effects that include affecting build packaging

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Dave Liebreich [:davel]]

I'd like to enable tests on all tinderboxes (or reasonable subset thereof) so that all branches and platforms are running tests.

simply removing --disable-test from the tinderbox mozconfigS has unfortunate side effects.

I'd like to figure out what we need to change, and make those changes, so that we can run the tests on the tinderboxes.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Maybe unit tests should be runnable even with --disable-tests?

Comment 2

[Dave Liebreich [:davel]]

(In reply to comment #1)
> Maybe unit tests should be runnable even with --disable-tests?
> 

Care to make a proof-of-concept patch?

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch?

something like this perhaps. untested though, and this patch only does netwerk/.

Comment 4

[Dave Liebreich [:davel]]

your patch does not move files out of dist/bin, which is one of the things we need to do.

Comment 5

[Dave Liebreich [:davel]]

benjamin asked about this bug in bug 354401.  we should probably get started on it sooner rather than later.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Move xpcshell tests/harness out of dist/bin

This works for me at making xpcshell tests not go in dist/bin.  Whether it's completely "right" (I doubt it is; path-passing dependencies in test_all.sh at a minimum come to mind) I leave up to more experienced build-fu masters.

This patch removes all use of dist/bin from xpcshell tests.  In doing so it is the beginning of the following layout of test stuff.  First, we add a new subdirectory of $(DIST) called testing.  This directory contains two subdirectories: 'harnesses' and 'tests'.  Harnesses will contain test harness code for execution; tests contains the tests.

$(DIST)/
- testing/
  - harnesses/
    ...harness files...
  - tests/
    ...test files...

The structure underneath $(DIST)/testing/harnesses consists of one directory per test harness.  For now, the only directory would be xpcshell-simple.  Long-term, however, we'd likely want to get mochitest and reftest stuff there, at least, and if other test harnesses emerge, they'd fit there just as easily.

$(DIST)/testing/harnesses/
- xpcshell-simple
- mochitest (eventually)
- reftest (eventually)

One interesting question: is there a good reason to move harnesses into dist/testing?  If we do so, config/rules.mk is less dependent upon the behavior of the test harnesses, and the harnesses have freedom to set up things however they'd like.  (Consider, for example, mochitest, which is toying with the idea of including thttpd as part of the suite [note to self: get the JS server working with mochitest] -- where will they compile it if they don't have something like this and can't use dist/bin?)  I think this is a reasonable win, but I could probably be convinced it's not enough of one; my time over the next several weeks is such that I may not have time for a revamp to handle that sort of change.  (On the other hand, I might.)

The structure underneath $(DIST)/testing/tests is a MODULE/TEST_SUITE/<test-type>/ hierarchy.  MODULE is the variable as set in the Makefile.  I'd prefer if the MODULE weren't a test_* module, but since that may have side effects (see netwerk/test/httpserver for one such side effect via IDL) we'll have to suck it up.  Perhaps a TEST_MODULE variable which, if not defined, is set to MODULE might work?  TEST_SUITE is intended to reduce the need for the current standard scheme of shoving every test in a single directory and then using that directory for tests; it would allow for tests to be split up as far as the directory structure allows.  (I don't see a way around the directory structure limitation without going back to manual test moving and invocation, which is a bad way to do things -- harder to change in the future.  Heck, this was tedious enough to change as is, and we barely have any tests.)  The <test-type> splits up tests by their type.  We could eliminate this last step if desired and leave conflicts up to the tester, but I'm not a huge fan.  Within this directory would be the files needed for the test, more or less.  (More on that later.)

Overall, then, the testing setup is as follows:

$(DIST)/testing
- harnesses
  - xpcshell-simple
    ...others...
- tests
  - content
    - dom3 (example, doesn't actually exist)
      - xpcshell-simple
        - test_foo.js
  - libjar
    - general
      - xpcshell-simple
        - test_bug333423.js
  - ...

To get xpcshell unit tests running, all you need do is define MODULE to some sort of module-name string, TEST_SUITE to a string describing what particular functionality is being tested, and XPCSHELL_TESTS_DIR to the name of the directory underneath the current Makefile where the xpcshell JS tests are located.  This is a big win over the current approach of manually doing everything, which makes it hard to change any aspect of it.

In doing the patch, I also tried to fix some of the issues that this move revealed.  There were a few problems.  First, tests were relying on where they were being run from to work, mostly to get access to local files for tests.  Second, some tests relied on dist/bin-based locations -- mostly as a result of my changes as part of bug 342877.

The way I fixed these issues is a bit of a hack.  Basically, I pass in all the locations to the test_all.sh script, which takes those values, determines parameters to pass to xpcshell, and seeds its environment appropriately.  It's of course somewhat fragile.  From there, I expose two functions in head.js for use by tests to get this information in a location-agnostic way: do_import_script is used to execute a JS file in the current test's context, with the topsrcdir-relative path as its argument, and do_get_srcdir gets a platform-dependent string specifying the location of the directory of tests being executed.  The former type of string is required for the xpcshell environment to be happy.  The latter, native type is required because tests which need other non-JS files will have to go through nsILocalFile, and that's not going to handle cygpaths, forward slashes, etc.  I agree -- it sucks.

This doesn't fix all the test dependencies on dist/bin -- there are still a lot of test binaries being copied into there, for a start -- but it's a good start, I think.  Feedback appreciated.

Comment 7

[Benjamin Smedberg]

Comment on attachment 248749 [details] [diff] [review]
Move xpcshell tests/harness out of dist/bin

>Index: config/rules.mk

I'd like to avoid pollution $(DIST) altogether: tests should go in $(DEPTH)/_tests

>-# This will strip out symbols that the component shouldnt be 
>+# This will strip out symbols that the component shouldn't be 

please make this "should not": various stupid editors get confused with mismatched quotes otherwise :-(

>+ifdef TEST_SUITE

I think we also want ifdef ENABLE_TESTS here, no?

>+ifdef XPCSHELL_TESTS_DIR
>+libs::
>+	@$(EXIT_ON_ERROR) \
>+        for jsfile in $(wildcard $(srcdir)/$(XPCSHELL_TESTS_DIR)/*.js); do \
>+		$(INSTALL) $$jsfile \
>+			$(DIST)/testing/tests/$(MODULE)/$(TEST_SUITE)/xpcshell-simple; \
>+	done
>+endif # XPCSHELL_TESTS_DIR

There's no need for a loop, you can install multiple files at once:

$(INSTALL) $(wildcard $(srcdir)/$(XPCSHELL_TESTS_DIR)/*.js) \
  $(DIST)/testing/tests/$(MODULE)/$(TEST_SUITE)/xpcshell-simple

>+# Test execution, after test files have been installed
>+
>+ifdef XPCSHELL_TESTS_DIR
>+ifdef CYGWIN_WRAPPER
>+NATIVE_SRCDIR := `cygpath -wa $(srcdir)/$(XPCSHELL_TESTS_DIR)`
>+else
>+NATIVE_SRCDIR := $(srcdir)/$(XPCSHELL_TESTS_DIR)
>+endif # CYGWIN_WRAPPER
>+endif # XPCSHELL_TESTS_DIR

Why do you have to set NATIVE_SRCDIR specially, instead of just using CYGWIN_WRAPPER in the command below?

Otherwise, this looks fine. If you don't have time to fix this patch, let me know and I'll try to find somebody to fix it, or find time to do it myself.

Comment 8

[Jeff Walden [:Waldo]]

I'm a little too busy at the moment to respond fully, but a tidbit for thought:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=sayrer%25gmail.com&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-12-19+11%3A50&maxdate=2006-12-19+11%3A59&cvsroot=%2Fcvsroot

This conflicts with the patch here -- assuming we go with the style this patch introduces, do we want to say that the places tests should really be broken up into different directories?

Comment 9

[Jeff Walden [:Waldo]]

(In reply to comment #7)
> Otherwise, this looks fine. If you don't have time to fix this patch, let me
> know and I'll try to find somebody to fix it, or find time to do it myself.

If you're fine waiting until at least mid-January, then I can fix it up; otherwise, feel free to attack with gusto.

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Use _tests directory, update to latest trunk

This moves the tests to $(DEPTH)/_tests/xpcshell-simple/$(MODULE)/$testdir, where $testdir is the name of the directory containing the tests.  The directory name is put in the variable XPCSHELL_TESTS, which is a list of directory names (to accommodate division of tests by functionality, if desired -- see browser/components/places/tests for the motivation for this.  This is tested on Linux and nowhere else currently.

I believe I've fixed the issues mentioned, except for the CYGWIN_WRAPPER comment -- I don't understand what's being asked there.

Comment 11

[Benjamin Smedberg]

Comment on attachment 253275 [details] [diff] [review]
Use _tests directory, update to latest trunk

>Index: config/rules.mk

>+libs::
>+	@$(EXIT_ON_ERROR) \
>+	for testdir in $(XPCSHELL_TESTS); do \
>+		$(INSTALL) \
>+			$(srcdir)/$$testdir/*.js \
>+			$(DEPTH)/_tests/xpcshell-simple/$(MODULE)/$$testdir; \
>+	done

Convention in this file is to use a single tab as required for rules, then spaces:

i.e. <tab>  $(INSTALL) \

>+ifdef XPCSHELL_TESTS
>+ifdef CYGWIN_WRAPPER
>+NATIVE_TOPSRCDIR := `cygpath -wa $(topsrcdir)`
>+else
>+NATIVE_TOPSRCDIR := $(topsrcdir)
>+endif # CYGWIN_WRAPPER

Is there any particular reason we can't get test_all.sh to do this, instead of doing it here?

>+else
>+RUN_XPCSHELL_TESTS =
>+endif # XPCSHELL_TESTS

No need to set it to empty, that's the default.

>+RUN_HEADLESS_TESTS = \
>+  @$(EXIT_ON_ERROR) \
>+  $(RUN_XPCSHELL_TESTS)
>+
>+endif # ENABLE_TESTS
>+
>+
>+# Testing targets
>+
>+check:: $(SUBMAKEFILES) $(MAKE_DIRS)
>+ifdef ENABLE_TESTS
>+	+$(RUN_HEADLESS_TESTS)
>+endif
>+	+$(LOOP_OVER_DIRS)
>+	+$(LOOP_OVER_TOOL_DIRS)

1) this will fail on some systems where "set -e" without a command after it kills the shell.
2) Why don't we just include the rule inside the "ifdef XPCSHELL_TESTS block" thus:

check::
<tab>@$(EXIT_ON_ERROR) $(RUN_XPCSHELL_TESTS)

That way we can skip the RUN_HEADLESS_TESTS variable entirely.

3) you definitely don't want the + in front of RUN_HEADLESS_TESTS anyway
4) please leave the check:: rule below which does the LOOP_OVER_DIRS bits. There's no need to combine the rules.

Otherwise, I love you!

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Now with even less suck!

(In reply to comment #11)
> Convention in this file is to use a single tab as required for rules, then
> spaces:

The entire file seems to be a mix of the two styles, and I think the one my patch had was the prevailing style, but I prefer fewer tabs anyway.  :-)


> Is there any particular reason we can't get test_all.sh to do this, instead of
> doing it here?

I don't think so, but I don't know the right way to determine whether we're cygwin from a shell script.  What this patch does is a hack I stole from somewhere else in the tree and may not be the least wrong way to do it.  (After fiddling with Makefiles and rules.mk here, I'm convinced that when it comes to our build system there's never a right way to do things, only lots of wrong ones.  Can we switch to Python?  :-P )


Are there any other less wrong things this patch could do?

Comment 13

[Benjamin Smedberg]

Comment on attachment 253360 [details] [diff] [review]
Now with even less suck!

ok, after talking with some people in #developers it seems that using cygpath in rules.mk is easier than trying to hack in in the shell. All the rest looks great.

And yes, we will be allowing python in our build system shortly, as soon as mozillabuild becomes the official windows build system.

Comment 14

[Jeff Walden [:Waldo]]

I finally (finally) managed to get the patch committed.  As I mentioned, it hadn't been tested anywhere but Linux, and then only on my machine, so I wasn't surprised there were problems.  The problems only affected non-mainline tinderboxen because --enable-tests isn't on those tinderboxen, tho, so I decided to do followup commits (only a few *wink*) to fix the problems, all of which basically stemmed from file path formats -- relativeness vs. absoluteness, cygpaths vs. Windows paths, forward slashes vs. backward slashes.  Here's the full log of commits:

http://bonsai.mozilla.org/cvsquery.cgi?who=jwalden%25mit.edu&whotype=match&sortby=Date&date=explicit&mindate=2007-01-30+19%3A25&maxdate=2007-01-31+21%3A50&cvsroot=%2Fcvsroot

This might look amusing, but believe me, debugging by tinderbox logs is *not* fun.  By the end, tho, there are really only a few important changes from the patch as posted (with the cygpath move back to config/rules.mk readded):

1)
Change do_get_topsrcdir() from returning a native path to returning an nsILocalFile.  Since the path as passed in the environment variable might be relative, doing this allows us to use the directory service to get the current working directory to resolve relative paths.  It also hides details of the path format from tests.  (Doing this meant changing all the files that used do_get_topsrcdir, but the changes were straightforward.)

However, this probably still isn't the best way -- I've filed bug 368942 to implement do_get_file(<path in topsrcdir>), obviating the need for appends that the current patch requires.

2)
Instead of passing in $(topsrcdir) directly, I pass it through cygpath to get an absolute, forward-slashed version.  This format works with load() in the jsshell and on the command line, and thus appended paths can all be forward-slashed.  The original unmodified version isn't adequate because it might be an absolute cygpath, which jsshell can't handle.  Even still, topsrcdir can be relative on Linux/Mac, so we have to get CurWorkD if the path isn't absolute.  (I further note that using appendRelativePath to do this shouldn't actually work according to IDL docs for nsILocalFile; I filed bug 368944 to correct this.)

3)
I changed embedding/Makefile.in and embedding/tests/Makefile.in so that embedding/tests wouldn't get built (as it wasn't built before).  Apparently the Windows code in there is broken and won't compile.  :-\  I'd file a bug except that it seems likely the code is going to be removed anyway.


I changed the xpcshell documentation to reflect the changes to the xpcshell environment:

http://developer.mozilla.org/en/docs/Writing_xpcshell-based_unit_tests

I'll add a dedicated page on XPCSHELL_TESTS to docs tomorrow, after I've gotten some sleep.  It should be a pretty short page -- MODULE must be defined, the variable itself must be set to a directory (or directories) containing xpcshell tests, and they have to be defined before config/rules.mk is included.


What else needs to be done with this bug so that we can --enable-tests?  There's certainly more beyond the xpcshell stuff, but I don't know what that more is.  (Also, I have one class which started this week and several more starting next week, so I'm going to be very busy pretty soon; progress here will probably slow down a bit.)

Comment 15

[Ray Kiddy]

Do you find that "make check" works? I have been trying things with a checked out and built version of Firefox and I checked some places and I do not find that it works. FYI, I am on an intel mac and fully acknowledge I may not be doing things right. But I do get runnable apps. But:

( cd mozilla/browser/components/places/tests ; make check )

AFAIK, this should run the tests:

browser/components/places/tests/testbookmarks.js
browser/components/places/tests/unit/test_annotations.js
browser/components/places/tests/unit/test_bookmarks.js
browser/components/places/tests/unit/test_history.js
browser/components/places/tests/unit/test_livemarks.js

Actually, this executes the annotations.js and test_history.js tests, fails on the second (with no info that I can see) and does not attempt the others.

Given work that is going on to run automated tests, is the "make check" approach still workable? I think it could be made to work, but not without an effort.

FYI, I have tried to identify which tests can be run with xpcshell. If one looks at the tests which "follow the rules", ie implement the run_test function, I find only the tests below. Are there others? Given that jssh is not usually built, can xpcshell be used as a replacement for jssh for other hjs files in the source? What might the reasons be to not do so?

netwerk/test/TestStandardURL.cpp
gfx/cairo/cairo/src/cairo-bentley-ottmann.c
netwerk/test/httpserver/test/test_basic_functionality.js
netwerk/test/httpserver/test/test_setindexhandler.js
netwerk/test/httpserver/test/test_errorhandler_exception.js
netwerk/test/httpserver/test/test_setstatusline.js
netwerk/test/httpserver/test/test_empty_body.js
netwerk/test/httpserver/test/test_headers.js
netwerk/test/httpserver/test/test_registerdirectory.js
netwerk/test/httpserver/test/test_default_index_handler.js
netwerk/test/httpserver/test/test_response_write.js
netwerk/test/unit/test_authentication.js
netwerk/test/unit/test_bug336501.js
netwerk/test/unit/test_bug331825.js
netwerk/test/unit/test_authpromptwrapper.js
netwerk/test/unit/test_bug337744.js
netwerk/test/unit/test_event_sink.js
netwerk/test/unit/test_content_sniffer.js
netwerk/test/unit/test_file_protocol.js
netwerk/test/unit/test_httpcancel.js
netwerk/test/unit/test_http_headers.js
netwerk/test/unit/test_cookie_header.js
netwerk/test/unit/test_protocolproxyservice.js
netwerk/test/unit/test_localstreams.js
netwerk/test/unit/test_idnservice.js
netwerk/test/unit/test_parse_content_type.js
xpcom/tests/unit/test_pipe.js
browser/components/places/tests/bookmarks/test_bookmarks.js
browser/components/places/tests/bookmarks/test_livemarks.js
browser/components/places/tests/unit/test_annotations.js
browser/components/places/tests/unit/test_history.js
parser/xml/test/unit/test_parser.js
content/test/unit/test_xml_serializer.js
content/test/unit/test_xml_parser.js
content/test/unit/test_isequalnode.js
content/test/unit/test_normalize.js
content/test/unit/test_nodelist.js
extensions/metrics/test/unit/test_event_item.js
modules/libjar/test/unit/test_bug333423.js
tools/test-harness/xpcshell-simple/example/unit/test_sample.js
embedding/tests/unit/test_wwpromptfactory.js
intl/uconv/tests/unit/test_bug321379.js
intl/uconv/tests/unit/test_charset_conversion.js

Comment 16

[Jeff Walden [:Waldo]]

(In reply to comment #15)
> ( cd mozilla/browser/components/places/tests ; make check )
> 
> AFAIK, this should run the tests:
> 
> browser/components/places/tests/testbookmarks.js
> browser/components/places/tests/unit/test_annotations.js
> browser/components/places/tests/unit/test_bookmarks.js
> browser/components/places/tests/unit/test_history.js
> browser/components/places/tests/unit/test_livemarks.js
> 
> Actually, this executes the annotations.js and test_history.js tests, fails on
> the second (with no info that I can see) and does not attempt the others.

Are you using an objdir?  This would only work if you aren't (and building without an objdir isn't recommended); if you are, you'd need to move to that directory in the objdir.  Also, you're looking at an old listing of files, prior to the reorganization of these files in the posted patch -- the annotations and history files should always be tested, but the *marks.js files should only be tested if MOZ_PLACES_BOOKMARKS or somesuch variable is set.  (testbookmarks.js should never be tested -- I don't know what the point of it is, to be honest.)


> Given work that is going on to run automated tests, is the "make check"
> approach still workable? I think it could be made to work, but not without an
> effort.

It works.  There are 5-10 tinderboxen running the tests every build, tho no mainline tinderboxen do.  Yet.


> FYI, I have tried to identify which tests can be run with xpcshell. If one
> looks at the tests which "follow the rules", ie implement the run_test
> function, I find only the tests below.

xpcshell tests are identified by containing a run_test function, being a JS file, and being in a directory whose parent directory contains a Makefile.in with XPCSHELL_TESTS being set to a space-separated list of directories, one of which is the name of the directory containing the test.  Most of the files you list fit this criteria; a very few do not.

> Given that jssh is not usually built, can xpcshell be used as a replacement
> for jssh for other hjs files in the source?

I'm not familiar with jssh.

Comment 17

[Benjamin Smedberg]

This is fixed, right?

Comment 18

[Jeff Walden [:Waldo]]

(In reply to comment #17)
> This is fixed, right?

I don't know.  Comment 4 implies that there's more to do (but reading through the bug I don't actually see anything listed), while bug 354401 comment 8 implies there may not be (no dist/bin, have Makefile variables for xpcshell tests).  If there's more to fix, perhaps we should close this bug, file a new one, and put the list in the first comment?


On an unrelated note, there's now an XPCSHELL_TESTS page on MDC.

Comment 19

[Rob Campbell [:rc] (:robcee)]

I'm all for closing this bug and opening up specific bugs for specific side-effects. ENABLE_TESTS works, it just dumps a little extra into dist/bin.

I'll check out the XPCSHELL_TESTS page. Thanks, Waldo!