If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

xpcshell tests should use relativesrcdir instead of $(MODULE)

RESOLVED FIXED in mozilla2.0b5

Status

Testing
XPCShell Harness
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jmaher, Assigned: jmaher)

Tracking

Trunk
mozilla2.0b5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 465345 [details] [diff] [review]
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.
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-
(Assignee)

Comment 3

7 years ago
Created attachment 465726 [details] [diff] [review]
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!
Attachment #465345 - Attachment is obsolete: true
Attachment #465726 - Flags: review?
(Assignee)

Updated

7 years ago
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+

Comment 5

7 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/d62bc37cb42e
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 6

7 years ago
Backed out: http://hg.mozilla.org/mozilla-central/rev/4192ba38ebee
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 7

7 years ago
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
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED

Comment 8

7 years ago
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 → ---
(Assignee)

Comment 9

7 years ago
Created attachment 469464 [details] [diff] [review]
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
Attachment #465726 - Attachment is obsolete: true

Comment 10

7 years ago
Landed - looks like third time is the charm: http://hg.mozilla.org/mozilla-central/rev/6b0184ebe03b
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → mozilla2.0b5
Version: unspecified → Trunk
(Assignee)

Comment 11

7 years ago
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
Blocks: 379327
Depends on: 395019
Duplicate of this bug: 394875
(Assignee)

Comment 13

7 years ago
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.

Comment 15

7 years ago
(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

7 years ago
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

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

Updated

7 years ago
Blocks: 593855
Created attachment 472561 [details] [diff] [review]
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
(Assignee)

Comment 19

7 years ago
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.

Updated

7 years ago
Attachment #472561 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.