322894 - unify necko and content unit tests

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Dave Liebreich [:davel]]

the unit test harnesses in mozilla/{content,netwerk}/test/unit/ are almost identical.  Extract common bits and put those bits in one place.

As a strawman, I propose new directory mozilla/js/src/xpconnect/harness, since the scripts are run using xpcshell.

Comment 1

[Benjamin Smedberg]

Brendan/shaver, does this directory choice sound reasonable to you?

Comment 2

[Boris Zbarsky [:bzbarsky]]

The harness depends on all sorts of contracts and interfaces that aren't part of xpconnect...  Perhaps we should just have a toplevel test directory or something?

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> The harness depends on all sorts of contracts and interfaces that aren't part
> of xpconnect...  Perhaps we should just have a toplevel test directory or
> something?

Right, we need something that doesn't misorder dependencies.  There is a partial order among modules, from more primitive to more derived, and xpconnect should not depend on more derived modules.

If the lowest common level at which to add that directory is mozilla/ -- and it seems that that's the case -- then so be it.  mozilla/harness?  Or would something with "unit" in the name be better?

/be

Comment 4

[Dave Liebreich [:davel]]

mozilla/tools/testharness/unit ?  I envision 2-3 harnesses co-existing, since one-size-does-not-fit-all.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> mozilla/tools/testharness/unit ?

Sounds good to me!

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #4)
> mozilla/tools/testharness/unit ?

The ghost of jwz says test-harness ;-).

/be

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yes, indeed, more cowbell^Whyphen.

Comment 8

[Dave Liebreich [:davel]]

Attachment: "diff" of new test harness files

this is a checkpoint - feel free to comment.

sample unit test in mozilla/tools/test-harness/xpcshell-simple/example/

In order for this to work, test_all.sh must exist in $(DIST)/bin/, and {head,tail}.js must exist in $(DIST)/bin/test-harness/xpcshell-simple/

I still need to learn what Makefile-fu needs to happen to include these files in checkouts, and do the above installs.

Comment 9

[Dave Liebreich [:davel]]

notes about attachment 208231 [details] [diff] [review]:
1) ^m2^mozilla^ in filenames
2) head-content.js contains the code that made content/test/unit/head.js different from netwerk/test/unit/head.js, and belongs in content/test/unit/ instead.  I'll put it there in a separate check-in when I modify that test harness instance to use the common files.

Comment 10

[Dave Liebreich [:davel]]

Please see descriptions of new file layout and info needed sections of http://wiki.mozilla.org/SoftwareTesting:Tools:Simple_xpcshell_test_harness - then please comment on this initial structure.

Thanks

_Dave

Comment 11

[Dave Liebreich [:davel]]

Any objection to adding mozilla/tools/test-harness/xpcshell-simple/ as the directory holding the unified test harness files?

If not, I'll go ahead and cvs add the dirs, then submit a patch for review.

Comment 12

[Boris Zbarsky [:bzbarsky]]

That seems reasonable.

Comment 13

[Dave Liebreich [:davel]]

Attachment: patch to add new test harness files

patch to add common harness files.  Another patch coming soon to modify content and necko instances of this harness to use the common files.

Comment 14

[Dave Liebreich [:davel]]

Attachment: patch including changes to existing instances of test harness

This patch fixes a minor issue with the previous version, and includes changes to necko and content instances of this harness to make them use the common files.

Comment 15

[Dave Liebreich [:davel]]

Comment on attachment 209422 [details] [diff] [review]
patch including changes to existing instances of test harness

requesting additional review from bz (creator of the content instance of this harness)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 209422 [details] [diff] [review]
patch including changes to existing instances of test harness

r=bzbarsky, though my makefile fu is very weak...  It all looks good as far as I can tell.  ;)

Comment 17

[Darin Fisher]

Comment on attachment 209422 [details] [diff] [review]
patch including changes to existing instances of test harness

Looks good, except I think you are missing necessary changes to allmakefiles.sh.  

r=darin with the appropriate changes to that file.

It may also be worth your while to ask bsmedberg to review this patch as well since he knows our build system the best.

Comment 18

[Dave Liebreich [:davel]]

Comment on attachment 209422 [details] [diff] [review]
patch including changes to existing instances of test harness

Benjamin, please review and let me know if additional changes are needed (for example, to allmakefiles.sh)

Comment 19

[Benjamin Smedberg]

Comment on attachment 209422 [details] [diff] [review]
patch including changes to existing instances of test harness

$(wildcard)s are in general frowned upon, but in this case I think they make sense. You just have to be aware that adding a new test automagically makes it part of the unit tests.

Comment 20

[Dave Liebreich [:davel]]

checked in to trunk

Comment 21

[Dave Liebreich [:davel]]

marking fixed as it has been checked in to the trunk.

Comment 22

[Dave Liebreich [:davel]]

Attachment: patch for 1.8 branch (ff2)

porting test harness consolidation patch so that places tests can use the harness.

Comment 23

[Darin Fisher]

Comment on attachment 210185 [details] [diff] [review]
patch for 1.8 branch (ff2)

r+a=darin

Comment 24

[Dave Liebreich [:davel]]

Checking in Makefile.in;
/cvsroot/mozilla/Makefile.in,v  <--  Makefile.in
new revision: 1.299.2.6; previous revision: 1.299.2.5
done
Checking in client.mk;
/cvsroot/mozilla/client.mk,v  <--  client.mk
new revision: 1.245.2.8; previous revision: 1.245.2.7
done
Checking in netwerk/test/Makefile.in;
/cvsroot/mozilla/netwerk/test/Makefile.in,v  <--  Makefile.in
new revision: 1.85.2.2; previous revision: 1.85.2.1
done
Removing netwerk/test/unit/head.js;
/cvsroot/mozilla/netwerk/test/unit/Attic/head.js,v  <--  head.js
new revision: delete; previous revision: 1.2
done
Removing netwerk/test/unit/tail.js;
/cvsroot/mozilla/netwerk/test/unit/Attic/tail.js,v  <--  tail.js
new revision: delete; previous revision: 1.1
done
Removing netwerk/test/unit/test_all.sh;
/cvsroot/mozilla/netwerk/test/unit/Attic/test_all.sh,v  <--  test_all.sh
new revision: delete; previous revision: 1.5
done
Removing netwerk/test/unit/test_sample.js;
/cvsroot/mozilla/netwerk/test/unit/Attic/test_sample.js,v  <--  test_sample.js
new revision: delete; previous revision: 1.1
done
Checking in tools/test-harness/Makefile.in;
/cvsroot/mozilla/tools/test-harness/Makefile.in,v  <--  Makefile.in
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in tools/test-harness/xpcshell-simple/Makefile.in;
/cvsroot/mozilla/tools/test-harness/xpcshell-simple/Makefile.in,v  <--  Makefile.in
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in tools/test-harness/xpcshell-simple/README;
/cvsroot/mozilla/tools/test-harness/xpcshell-simple/README,v  <--  README
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in tools/test-harness/xpcshell-simple/head.js;
/cvsroot/mozilla/tools/test-harness/xpcshell-simple/head.js,v  <--  head.js
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in tools/test-harness/xpcshell-simple/tail.js;
/cvsroot/mozilla/tools/test-harness/xpcshell-simple/tail.js,v  <--  tail.js
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in tools/test-harness/xpcshell-simple/test_all.sh;
/cvsroot/mozilla/tools/test-harness/xpcshell-simple/test_all.sh,v  <--  test_all.sh
new revision: 1.2.2.2; previous revision: 1.2.2.1
done
Checking in tools/test-harness/xpcshell-simple/example/Makefile.in;
/cvsroot/mozilla/tools/test-harness/xpcshell-simple/example/Makefile.in,v  <--  Makefile.in
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in tools/test-harness/xpcshell-simple/example/unit/test_sample.js;
/cvsroot/mozilla/tools/test-harness/xpcshell-simple/example/unit/test_sample.js,v  <--  test_sample.js
new revision: 1.1.2.2; previous revision: 1.1.2.1
done  

Comment 25

[Dave Liebreich [:davel]]

I'm going to back out the changes to mozilla/netwerk/test/unit because tinderbox client scripts invoke the necko tests uing test_all.sh in dist/bin/necko_unit_tests and I'm not sure how to update the tinderbox client scripts on the build machines.  The harness common files will remain.

Comment 26

[Dave Liebreich [:davel]]

Checking in Makefile.in;
/cvsroot/mozilla/netwerk/test/Makefile.in,v  <--  Makefile.in
new revision: 1.85.2.3; previous revision: 1.85.2.2
done
Checking in unit/head.js;
/cvsroot/mozilla/netwerk/test/unit/Attic/head.js,v  <--  head.js
new revision: 1.2.8.2; previous revision: 1.2.8.1
done
Checking in unit/tail.js;
/cvsroot/mozilla/netwerk/test/unit/Attic/tail.js,v  <--  tail.js
new revision: 1.1.8.2; previous revision: 1.1.8.1
done
Checking in unit/test_all.sh;
/cvsroot/mozilla/netwerk/test/unit/Attic/test_all.sh,v  <--  test_all.sh
new revision: 1.5.2.2; previous revision: 1.5.2.1
done   

Comment 27

[Mikhail T.]

How is one supposed to run these tests?

When I try to:

cd mozilla/dist/bin/necko_unit_tests
./test_all.sh

They all fail with the ``failed to get nsJSRuntimeService!'' complaint from xpcshell...

Thanks!

Comment 28

[Boris Zbarsky [:bzbarsky]]

> How is one supposed to run these tests?

In the objdir:

  cd netwerk
  make check