Convert Makefiles to use |TEST_DIRS += foo| instead of |ifdef ENABLE_TESTS \n DIRS += foo|

RESOLVED FIXED in mozilla13

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: gps, Assigned: Panagiotis Koutsourakis, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Now that we have a TEST_DIRS variable (bug 701822), the Makefiles should make use of it.

Basically, this involves replacing:

  ifdef ENABLE_TESTS
  DIRS += tests
  endif

With

  TEST_DIRS += tests

Since I created this mess, I'll mop it up. Not sure when I'll get to it though. If someone wants to have a go at it, please do!

Comment 1

6 years ago
For anyone wanting to do this as a first bug:

1) Get mozilla-central:
   https://developer.mozilla.org/En/Developer_Guide/Source_Code/Mercurial

2) Make the change mentioned in comment 0 for each of the search results here:
   http://mxr.mozilla.org/mozilla-central/search?string=ifdef+ENABLE_TESTS&find=Makefile\.in

3) Check everything still builds ok:
   https://developer.mozilla.org/En/Simple_Firefox_build
   ...NB: Make sure bug 701822 has been marked as fixed (ie has merged to mozilla-central), since until it does, TEST_DIRS won't work.

4) Create a patch to attach here:
   https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch
   https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3f

5) Set the r? flag to request review from Kyle Huey when you attach the patch (type in :khuey and it will auto-complete).

If you have any questions (or would like to take on this bug and don't have the needed bugzilla permissions to assign it to yourself), just ask! :-)
Assignee: gps → nobody
Summary: Convert Makefiles to use TEST_DIRS → Convert Makefiles to use |TEST_DIRS += foo| instead of |ifdef ENABLE_TESTS \n DIRS += foo|
Whiteboard: [good first bug] [mentor=edmorley]
(Reporter)

Comment 2

6 years ago
Looks like there is a proposal for MOCHITESTS_DIR in bug 370750. If that bug lands, that will impact this one.
Depends on: 370750
(Reporter)

Updated

6 years ago
Blocks: 697894
(In reply to Gregory Szorc [:gps] from comment #2)
> Looks like there is a proposal for MOCHITESTS_DIR in bug 370750

Note that the latest patch in that bug uses creates *_FILES, not *_DIR, so it's somewhat orthogonal.
(Assignee)

Comment 4

5 years ago
(In reply to Ed Morley [:edmorley] from comment #1)

> 2) Make the change mentioned in comment 0 for each of the search results
> here:
>   
> http://mxr.mozilla.org/mozilla-central/
> search?string=ifdef+ENABLE_TESTS&find=Makefile\.in
> 

Most of the files in that set do not contain the string 
  DIRS += tests 
(I found only 27). For instance rdf/Makefile.in has
  TOOL_DIRS += tests
I guess all of them should be changed, but I'd like to verify this first.

Also I'm not sure how I can take ownership of this bug. I just created an account so if someone would be kind enough to help me with this I would appreciate it very much.
(Reporter)

Comment 5

5 years ago
(In reply to Panagiotis Koutsourakis from comment #4)
> Also I'm not sure how I can take ownership of this bug. I just created an
> account so if someone would be kind enough to help me with this I would
> appreciate it very much.

The bug is is assigned to "nobody," which means it is free for someone to take. If you are intent on addressing this bug, assign the bug to yourself.
(Reporter)

Comment 6

5 years ago
Doh. Apparently new Bugzilla users can't assign bugs to themselves. So, if you want to do the work for this bug, just say so and someone (likely this bug's mentor - Ed Morley) will assign it to you. Sorry for the mix-up.
(Assignee)

Comment 7

5 years ago
(In reply to Gregory Szorc [:gps] from comment #6)
> Doh. Apparently new Bugzilla users can't assign bugs to themselves. So, if
> you want to do the work for this bug, just say so and someone (likely this
> bug's mentor - Ed Morley) will assign it to you. Sorry for the mix-up.

No problem. Yes, I'd like to work on it. Thanks for the help.

Updated

5 years ago
Assignee: nobody → panagiotis.koutsourakis
(Assignee)

Comment 8

5 years ago
Created attachment 590455 [details] [diff] [review]
Change Makefiles to use the variable TEST_DIRS

As mentioned in the commit message some files that contain the string

ifdef ENABLE_TESTS remain unchanged by this patch.

The source compiled with no problems under GNU/Linux 3.0.6 with gcc 4.5.3. Unfortunately I don't have access to other platforms (or the try server), so I could not test if it compiles in other platforms.

If further changes are needed, I'd be happy to help.
Attachment #590455 - Flags: review?(khuey)

Comment 9

5 years ago
(In reply to Panagiotis Koutsourakis from comment #8)
> The source compiled with no problems under GNU/Linux 3.0.6 with gcc 4.5.3.
> Unfortunately I don't have access to other platforms (or the try server), so
> I could not test if it compiles in other platforms.

Sent to try:
https://tbpl.mozilla.org/?tree=Try&rev=0856745e4ed2

:-)
Comment on attachment 590455 [details] [diff] [review]
Change Makefiles to use the variable TEST_DIRS

Review of attachment 590455 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Thanks for the patch.
Attachment #590455 - Flags: review?(khuey) → review+
Sorry for the delay, this got lost in my bugmails.

Have pushed to mozilla-inbound, which will get merged to mozilla-central within the next 24 hours.

https://hg.mozilla.org/integration/mozilla-inbound/rev/064aba8b26f2

Thanks for the patch! :-)
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla13
Merged to mozilla-central:
https://hg.mozilla.org/mozilla-central/rev/064aba8b26f2

If you'd like any inspiration for things to work on next - pop onto #developers on irc.mozilla.org and we'll gladly find something for you to work on :-)
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

5 years ago
Thanks :) I'll take you up on that :)

Comment 14

5 years ago
There also  some in layout\Makefile.in.

Er, Should change it?
(Assignee)

Comment 15

5 years ago
Created attachment 595341 [details] [diff] [review]
Convert some more files ommited in the previous patch.

As per our discussion with Kyle in #developers I did not change layout/Makefile.in
Attachment #595341 - Flags: review?(khuey)
Also, I've just spotted that bug 294260 comment 53 (cset 21cc56ed5fe6) removed a TEST_DIRS accidentally :-(

(Reopening for the second patch).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I've said to Matt that you would be ok including the comment 16 change in your followup if that's ok? :-)
(Assignee)

Comment 18

5 years ago
Created attachment 595690 [details] [diff] [review]
Convert some more files ommited in the previous patch.

This patch includes the file mentioned in #comment 16.
Attachment #595341 - Attachment is obsolete: true
Attachment #595341 - Flags: review?(khuey)
Attachment #595690 - Flags: review?(khuey)
(Assignee)

Comment 19

5 years ago
Sent to try server: https://tbpl.mozilla.org/?tree=Try&rev=6c114e3b8c55
Comment on attachment 595690 [details] [diff] [review]
Convert some more files ommited in the previous patch.

Review of attachment 595690 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #595690 - Flags: review?(khuey) → review+
TBPL looks good to me (I haven't starred, since I thought you might want to get used to it :-))
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f30e16bf02d
https://hg.mozilla.org/mozilla-central/rev/1f30e16bf02d
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Blocks: 735433

Updated

3 years ago
Mentor: emorley@mozilla.com
Whiteboard: [good first bug] [mentor=edmorley] → [good first bug]
You need to log in before you can comment on or make changes to this bug.