593855 - Port |Bug 586754 Xpcshell tests should use relativesrcdir instead of $MODULE| to comm-central

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[Justin Wood (:Callek)]

We should make sure we are adjusted for the test moves from Bug 586754 as well as make our system do the same thing (rel-path).

Comment 1

[Justin Wood (:Callek)]

Attachment: Part 1, actually find and use the places file [checkin: c#10]

Comment 2

[Justin Wood (:Callek)]

Attachment: Part 2, Adds relpath to Makefiles that need them

This patch adds relpath to the Makefiles that will need them for Part 3.

Comment 3

[Justin Wood (:Callek)]

Attachment: Part 5, remove MODULE from dirs that no longer need it

After Parts 3 and 4 of this bug, We won't need MODULE in these dirs at all.

Comment 4

[Justin Wood (:Callek)]

All patches here are independant of each other, except Patch 3 and 4 which I will attach only after I verify that it will pass c-c try.

Part 3 is the rules.mk Build System Changes
Part 4 is teaching the c-c test system about the new dir structure.

Comment 5

[Justin Wood (:Callek)]

Attachment: Part 4, fixup paths (v1)

I have a feeling I may have missed something, won't request review until after it runs through try.

Comment 6

[Justin Wood (:Callek)]

Attachment: Part 3, rules.mk changes

I have rs+ from both Mitch and Khuey to kill the $(error) part here, that was just commented-out in m-c, on that side. So I didn't sync that.

Comment 7

[Justin Wood (:Callek)]

Attachment: Part 3, rules.mk changes (v1)

--err-- non-empty-cset patch

Comment 8

[Justin Wood (:Callek)]

Attachment: Part 2, Adds relpath to Makefiles that need them (v2)

...corrected after findings from local testing

Comment 9

[Justin Wood (:Callek)]

Attachment: Part 4, fixup paths (v2)

Ready for review...

Notes:
* I tested locally (tb-try doesn't run unit tests)
* gRelDir is used due to some test dirs now having different relPaths than other test Dirs, but still loading the same files, which in turn load other files... so this is needed to let the tests find the right stuff.
* one test I had to remove a duplicate relpath thing, due to it actually working by accident before, (with the new depths it was expanding wrong with my patch)
* Mark said you (KaiRo) could review this as long as it was tested, simply requesting feedback as a quick "look at this" from him, due to the extent of changes and that I added a new global var a few places.

Comment 10

[Justin Wood (:Callek)]

Comment on attachment 472448 [details] [diff] [review]
Part 1, actually find and use the places file [checkin: c#10]

Pushed Part 1: http://hg.mozilla.org/comm-central/rev/3a5ccff4af38

Comment 11

[Justin Wood (:Callek)]

Comment on attachment 472558 [details] [diff] [review]
Part 4, fixup paths (v2)

had to add suite/ head_bookmarks.js to the list of paths I adjusted, did not update patch of course.

Comment 12

[Robert Kaiser]

Comment on attachment 472557 [details] [diff] [review]
Part 2, Adds relpath to Makefiles that need them (v2)

>diff --git a/mailnews/test/performance/bloat/Makefile.in b/mailnews/test/performance/bloat/Makefile.in
>+relativesrcdir = mailnews/performance/bloat


Somewhat funny that you removed the test/ part from the path here, but left it in all cases where it's at the end...

Comment 13

[Robert Kaiser]

Comment on attachment 472558 [details] [diff] [review]
Part 4, fixup paths (v2)

>+gRelDepth = "../../../../";

I'd slightly prefer gDEPTH as the var name so it matches more closely the var we already have in Makefiles that basically does the same - but then, that's bikeshedding, and probably something Mark or someone else from the "mailnews dept." should decide.

>diff --git a/mailnews/base/test/unit/test_postPluginFilters.js b/mailnews/base/test/unit/test_postPluginFilters.js
> function getSpec(aFileName)
> {
>-  var file = do_get_file("../../mailnews/data/" + aFileName);
>+  var file = do_get_file(aFileName);

So it now magically knows where the data/ dir is?

>diff --git a/mailnews/test/resources/messageInjection.js b/mailnews/test/resources/messageInjection.js
>     // -- Pull in the POP3 fake-server / local account helper code
>-    load("../../test_mailnewslocal/unit/head_maillocal.js");
>+    load(gRelDepth + "mailnews/local/test/unit/head_maillocal.js");

Here you don't need to ensure that the depth is set, like you do in the pumps?

I didn't actually do a test run myself, just inspected the code, but I trust you did a full run - and IIRC you also did a try run, right?

Comment 14

[Robert Kaiser]

Comment on attachment 472448 [details] [diff] [review]
Part 1, actually find and use the places file [checkin: c#10]

>diff --git a/suite/common/places/tests/unit/head_bookmarks.js b/suite/common/places/tests/unit/head_bookmarks.js
> // Import common head.
>-let (commonFile = do_get_file("../../test_places/head_common.js", false)) {
>+let (commonFile = do_get_file("../../toolkit/components/places/tests/head_common.js", false)) {

Actually, is that relative path still correct _after_ all your changes?

Comment 15

[Justin Wood (:Callek)]

(In reply to comment #12)
> Comment on attachment 472557 [details] [diff] [review]
> Part 2, Adds relpath to Makefiles that need them (v2)
> 
> >diff --git a/mailnews/test/performance/bloat/Makefile.in b/mailnews/test/performance/bloat/Makefile.in
> >+relativesrcdir = mailnews/performance/bloat
> 
> 
> Somewhat funny that you removed the test/ part from the path here, but left it
> in all cases where it's at the end...

Whops, fixed.

(In reply to comment #13)
> Comment on attachment 472558 [details] [diff] [review]
> Part 4, fixup paths (v2)
> 
> >+gRelDepth = "../../../../";
> 
> I'd slightly prefer gDEPTH as the var name 

I'm inclined to agree with you, so not exactly a bikeshed, and I'll fix (uses of that are minimal)

> >diff --git a/mailnews/base/test/unit/test_postPluginFilters.js b/mailnews/base/test/unit/test_postPluginFilters.js
> > function getSpec(aFileName)
> > {
> >-  var file = do_get_file("../../mailnews/data/" + aFileName);
> >+  var file = do_get_file(aFileName);
> 
> So it now magically knows where the data/ dir is?

Not exactly, the |aFileName| is already something like "../../../data" the reason this worked before was "../../mailnews/data/../../mailnews/data" still expanded to the same dir. (See the filename array near the top of the file)

> >diff --git a/mailnews/test/resources/messageInjection.js b/mailnews/test/resources/messageInjection.js
> >     // -- Pull in the POP3 fake-server / local account helper code
> >-    load("../../test_mailnewslocal/unit/head_maillocal.js");
> >+    load(gRelDepth + "mailnews/local/test/unit/head_maillocal.js");
> 
> Here you don't need to ensure that the depth is set, like you do in the pumps?

I probably should do it in this file too, yea. Thanks.

> I didn't actually do a test run myself, just inspected the code, but I trust
> you did a full run - and IIRC you also did a try run, right?

I did a test run on TB since that is where basically all these tests run (didn't run on SeaMonkey), and as I worked on correcting the mistakes did it in mailnews/ itself.

Try for TB only runs mozmill, not xpcshell sadly.

(In reply to comment #14)
> Comment on attachment 472448 [details] [diff] [review]
> Part 1, actually find and use the places file [checkin: c#10]
> 
> >diff --git a/suite/common/places/tests/unit/head_bookmarks.js b/suite/common/places/tests/unit/head_bookmarks.js
> Actually, is that relative path still correct _after_ all your changes?

No its, not; but I did fix it locally (see c#11)

Comment 16

[Robert Kaiser]

(In reply to comment #15)
> I did a test run on TB since that is where basically all these tests run
> (didn't run on SeaMonkey), and as I worked on correcting the mistakes did it in
> mailnews/ itself.

Please do a SeaMonkey run as well before checking in.

> Try for TB only runs mozmill, not xpcshell sadly.

Oh, that's unfortunate.

> (In reply to comment #14)
> No its, not; but I did fix it locally (see c#11)

Ah, OK, I thought so. :)

Comment 17

[Justin Wood (:Callek)]

Part 2: http://hg.mozilla.org/comm-central/rev/0f7cc68b59a7
Part 3: http://hg.mozilla.org/comm-central/rev/4380f07f155d
Part 4: http://hg.mozilla.org/comm-central/rev/a09c8dfad8a9
Part 5: http://hg.mozilla.org/comm-central/rev/8e3d1110643b

Followup: http://hg.mozilla.org/comm-central/rev/189d8a612532
MozMill Followup: http://hg.mozilla.org/comm-central/rev/0d8a88a129ca

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 472558 [details] [diff] [review]
Part 4, fixup paths (v2)

This seems to be working fine ;-)