Closed Bug 322894 Opened 15 years ago Closed 15 years ago

unify necko and content unit tests

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: davel, Assigned: davel)

References

Details

(Keywords: fixed1.8.1)

Attachments

(2 files, 2 obsolete files)

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.
Brendan/shaver, does this directory choice sound reasonable to you?
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?
(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
mozilla/tools/testharness/unit ?  I envision 2-3 harnesses co-existing, since one-size-does-not-fit-all.
> mozilla/tools/testharness/unit ?

Sounds good to me!
(In reply to comment #4)
> mozilla/tools/testharness/unit ?

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

/be
Yes, indeed, more cowbell^Whyphen.
Attached patch "diff" of new test harness files (obsolete) — Splinter Review
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.
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.
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
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.

That seems reasonable.
patch to add common harness files.  Another patch coming soon to modify content and necko instances of this harness to use the common files.
Attachment #209327 - Flags: review?
Attachment #209327 - Flags: review?
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.
Attachment #208231 - Attachment is obsolete: true
Attachment #209327 - Attachment is obsolete: true
Attachment #209422 - Flags: review?(darin)
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)
Attachment #209422 - Flags: review?(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.  ;)
Attachment #209422 - Flags: review?(bzbarsky) → review+
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.
Attachment #209422 - Flags: review?(darin) → review+
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)
Attachment #209422 - Flags: superreview?(benjamin)
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.
Attachment #209422 - Flags: superreview?(benjamin) → superreview+
checked in to trunk
marking fixed as it has been checked in to the trunk.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
porting test harness consolidation patch so that places tests can use the harness.
Attachment #210185 - Flags: review?(darin)
Attachment #210185 - Flags: branch-1.8.1?(darin)
Comment on attachment 210185 [details] [diff] [review]
patch for 1.8 branch (ff2)

r+a=darin
Attachment #210185 - Flags: review?(darin)
Attachment #210185 - Flags: review+
Attachment #210185 - Flags: branch-1.8.1?(darin)
Attachment #210185 - Flags: branch-1.8.1+
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  
Keywords: fixed1.8.1
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.
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   
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!
> How is one supposed to run these tests?

In the objdir:

  cd netwerk
  make check
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.