Closed Bug 586754 Opened 9 years ago Closed 9 years ago

xpcshell tests should use relativesrcdir instead of $(MODULE)

Categories

(Testing :: XPCShell Harness, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla2.0b5

People

(Reporter: jmaher, Assigned: jmaher)

References

Details

Attachments

(1 file, 3 obsolete files)

this will make it easier to use manifests between objdir and test packaging.

I have tested on try and seen green.  Found a few tests I was missing (by printing out all tests discovered with and without patch and doing a diff on my local linux build).  This current patch is running on try as there are 3 small differences.
Attachment #465345 - Flags: review?(ted.mielczarek)
Assignee: nobody → jmaher
Comment on attachment 465345 [details] [diff] [review]
change xpcshell makefiles from $(MODULE) to $(relativesrcdir)

Punting review to Mitch.
Attachment #465345 - Flags: review?(ted.mielczarek) → review?(mitchell.field)
Comment on attachment 465345 [details] [diff] [review]
change xpcshell makefiles from $(MODULE) to $(relativesrcdir)

>--- a/intl/locale/src/unix/tests/Makefile.in
>+++ b/intl/locale/src/unix/tests/Makefile.in
>@@ -34,15 +34,16 @@
> # the terms of any one of the MPL, the GPL or the LGPL.
> #
> # ***** END LICENSE BLOCK *****
> 
> DEPTH		= ../../../../..
> topsrcdir	= @top_srcdir@
> srcdir		= @srcdir@
> VPATH		= @srcdir@
>+relativesrcdir  = intl/locale/src/unit
> 
> include $(DEPTH)/config/autoconf.mk
> 
> MODULE          = test_intl_locale_unix
> XPCSHELL_TESTS  = unit
> 
> include $(topsrcdir)/config/rules.mk

relativesrcdir = intl/locale/src/unix/tests

>diff --git a/intl/locale/tests/Makefile.in b/intl/locale/tests/Makefile.in
>--- a/intl/locale/tests/Makefile.in
>+++ b/intl/locale/tests/Makefile.in
>@@ -34,15 +34,16 @@
> # the terms of any one of the MPL, the GPL or the LGPL.
> #
> # ***** END LICENSE BLOCK *****
> 
> DEPTH		= ../../..
> topsrcdir	= @top_srcdir@
> srcdir		= @srcdir@
> VPATH		= @srcdir@
>+relativesrcdir  = intl/local/tests
> 
> include $(DEPTH)/config/autoconf.mk
> 
> MODULE          = test_intl_locale
> XPCSHELL_TESTS  = unit
> 
> include $(topsrcdir)/config/rules.mk

relativesrcdir = intl/locale/tests

>diff --git a/intl/locale/tests_multilocale/Makefile.in b/intl/locale/tests_multilocale/Makefile.in
>--- a/intl/locale/tests_multilocale/Makefile.in
>+++ b/intl/locale/tests_multilocale/Makefile.in
>@@ -34,15 +34,16 @@
> # the terms of any one of the MPL, the GPL or the LGPL.
> #
> # ***** END LICENSE BLOCK *****
> 
> DEPTH		= ../../..
> topsrcdir	= @top_srcdir@
> srcdir		= @srcdir@
> VPATH		= @srcdir@
>+relativesrcdir	= int/local/tests_multilocale
> 
> include $(DEPTH)/config/autoconf.mk
> 
> MODULE          = test_intl_locale_multilocale
> XPCSHELL_TESTS  = unit
> 
> include $(topsrcdir)/config/rules.mk

relativesrcdir = intl/locale/tests_multilocale

>diff --git a/netwerk/test/httpserver/Makefile.in b/netwerk/test/httpserver/Makefile.in
>--- a/netwerk/test/httpserver/Makefile.in
>+++ b/netwerk/test/httpserver/Makefile.in
>@@ -37,16 +37,17 @@
> # ***** END LICENSE BLOCK *****
> 
> $(warning httpserver XPI_NAME=$(XPI_NAME))
> 
> DEPTH		= ../../..
> topsrcdir	= @top_srcdir@
> srcdir		= @srcdir@
> VPATH		= @srcdir@
>+relativesrcdir  = network/test/httpserver
> 
> include $(DEPTH)/config/autoconf.mk
> 
> MODULE          = test_necko
> NO_INTERFACES_MANIFEST = 1
> 
> EXTRA_COMPONENTS = \
>                    httpd.js \

relativesrcdir = netwerk/test/httpserver

Use a single space after relativesrcdir in places where you currently have "relativesrcdir  = whatever".
Attachment #465345 - Flags: review?(mitchell.field) → review-
updated patch with feedback from r-.  Thanks for the feedback and for reviewing this updated patch!
Attachment #465345 - Attachment is obsolete: true
Attachment #465726 - Flags: review?
Attachment #465726 - Flags: review? → review?(mitchell.field)
Comment on attachment 465726 [details] [diff] [review]
change xpcshell makefiles from $(MODULE) to $(relativesrcdir) (2.0)

Thanks for the patch!
Attachment #465726 - Flags: review?(mitchell.field) → review+
Landed: http://hg.mozilla.org/mozilla-central/rev/d62bc37cb42e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Backed out: http://hg.mozilla.org/mozilla-central/rev/4192ba38ebee
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Landed again with more sticking power (runs on try server showed a different patch to be the cause of the orange that bounced this last time): http://hg.mozilla.org/mozilla-central/rev/49beef9387a0
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
We got torpedoed by a change to xpcshell tests that happened between our try server run and our checkin: backed out: changeset: b5567c5bbda9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
updated for bitrot, new test that had a hardcoded module name in:
js/jetpack/tests/unit/test_jetpack_ctypes.js
Attachment #465726 - Attachment is obsolete: true
Landed - looks like third time is the charm: http://hg.mozilla.org/mozilla-central/rev/6b0184ebe03b
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → mozilla2.0b5
Version: unspecified → Trunk
In this patch (config/rules.mk) there are some lines commented out that should have been removed:
+#ifndef MODULE
+#$(error Must define MODULE when defining XPCSHELL_TESTS.)
+#endif
Depends on: 395019
Duplicate of this bug: 394875
the cleanup of config/rules.mk is in the patch for bug 591325
Why aren't we making the build fail when relativesrcdir is not set as we used to when MODULE wasn't set?

We have tests with no relativesrcdir set right now and they all get clumped together making for some surprising results when you only want to run one directory of tests.
(In reply to comment #14)
> Why aren't we making the build fail when relativesrcdir is not set as we used
> to when MODULE wasn't set?
> 
This is in the patch on bug 591325.

> We have tests with no relativesrcdir set right now and they all get clumped
> together making for some surprising results when you only want to run one
> directory of tests.
We don't have anything tracking this test clean up step yet (AFAIK).  Would you mind filing it and listing the tests that need to be changed?
Why did this land with the obvious regression? Can it be backed out until it is fixed properly? Adding the $(error) as Dave suggested would certainly find all the locations where relativesrcdir is not set, as well as preventing developers from creating more problems.
(In reply to comment #16)
> Why did this land with the obvious regression? Can it be backed out until it is
> fixed properly? Adding the $(error) as Dave suggested would certainly find all
> the locations where relativesrcdir is not set, as well as preventing developers
> from creating more problems.
It landed with the regression as neither the lander (myself) nor the reviewer realized it was a regression.  I'm sorry, it won't happen again.

The other patch that contains the fix to this oversight (bug 591325) is ready to go, why don't I land that right away?  Or I could break out the necessary changes out of that patch and land them as a bustage fix to this.
Blocks: 593855
Attached patch followup patch (obsolete) — Splinter Review
This is a followup patch to what landed, has rs+=khuey and Mitch via IRC. I'll just push it whenever I next push to m-c
actually we want to error out if 'relativesrcdir' is not defined.  This prevents us from missing tests or parts of tests.  I have a patchin bug 591325 that addresses this.
Attachment #472561 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.