586754 - xpcshell tests should use relativesrcdir instead of $(MODULE)

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: change xpcshell makefiles from $(MODULE) to $(relativesrcdir)

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.

Comment 1

[(not currently active) Ted Mielczarek]

Comment on attachment 465345 [details] [diff] [review]
change xpcshell makefiles from $(MODULE) to $(relativesrcdir)

Punting review to Mitch.

Comment 2

[Mitchell Field [:Mitch]]

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".

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: change xpcshell makefiles from $(MODULE) to $(relativesrcdir) (2.0)

updated patch with feedback from r-.  Thanks for the feedback and for reviewing this updated patch!

Comment 4

[Mitchell Field [:Mitch]]

Comment on attachment 465726 [details] [diff] [review]
change xpcshell makefiles from $(MODULE) to $(relativesrcdir) (2.0)

Thanks for the patch!

Comment 5

[cmtalbert]

Landed: http://hg.mozilla.org/mozilla-central/rev/d62bc37cb42e

Comment 6

[cmtalbert]

Backed out: http://hg.mozilla.org/mozilla-central/rev/4192ba38ebee

Comment 7

[cmtalbert]

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

Comment 8

[cmtalbert]

We got torpedoed by a change to xpcshell tests that happened between our try server run and our checkin: backed out: changeset: b5567c5bbda9

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: change xpcshell makefiles from $(MODULE) to $(relativesrcdir) (2.1)

updated for bitrot, new test that had a hardcoded module name in:
js/jetpack/tests/unit/test_jetpack_ctypes.js

Comment 10

[cmtalbert]

Landed - looks like third time is the charm: http://hg.mozilla.org/mozilla-central/rev/6b0184ebe03b

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

the cleanup of config/rules.mk is in the patch for bug 591325

Comment 14

[Dave Townsend [:mossop]]

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.

Comment 15

[cmtalbert]

(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?

Comment 16

[Benjamin Smedberg]

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.

Comment 17

[cmtalbert]

(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.

Comment 18

[Justin Wood (:Callek)]

Attachment: followup patch

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

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

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.