Last Comment Bug 701822 - Add TEST_DIRS Variable
: Add TEST_DIRS Variable
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Gregory Szorc [:gps]
:
Mentors:
Depends on:
Blocks: 702388 735433 759013
  Show dependency treegraph
 
Reported: 2011-11-11 12:35 PST by Gregory Szorc [:gps]
Modified: 2012-05-27 20:34 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Roll TEST_DIRS into DIRS variable (1.24 KB, patch)
2011-11-11 12:35 PST, Gregory Szorc [:gps]
ted: review-
Details | Diff | Splinter Review
Roll TEST_DIRS into DIRS variable, v2 (1.61 KB, patch)
2011-11-11 13:08 PST, Gregory Szorc [:gps]
ted: review+
Details | Diff | Splinter Review

Description Gregory Szorc [:gps] 2011-11-11 12:35:12 PST
Created attachment 573894 [details] [diff] [review]
Roll TEST_DIRS into DIRS variable

In the spirit of making our Makefiles more declarative and data-driven, I'd like to introduce the TEST_DIRS variable.

Instead of the current convention:

  ifdef ENABLE_TESTS
  DIRS += tests
  endif

I'd just like to say:

  TEST_DIRS += tests

The attached patch is very crude, but gets the job done. I could treat it like PARALLEL_DIRS and TOOL_DIRS and other variables and give it all kinds of targets, etc, but I think that is overkill at this time. If we want to treat it specially later, we can cross that bridge when we come to it.

If this patch is accepted, I'll submit a follow-up patch that converts the existing Makefiles to use the new convention (when I have time).

The attached patch is in Git format. I'll do the right thing for a Mercurial commit. Or, I can resubmit, if requested.
Comment 1 Ted Mielczarek [:ted.mielczarek] 2011-11-11 12:40:15 PST
Comment on attachment 573894 [details] [diff] [review]
Roll TEST_DIRS into DIRS variable

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

I have two qualms with this patch. (But I like the idea.)

::: config/rules.mk
@@ +121,5 @@
> +# Add test directories to the regular directories list. TEST_DIRS should
> +# arguably have the same status as TOOL_DIRS and other *_DIRS variables. It is
> +# coded this way until Makefiles stop using the "ifdef ENABLE_TESTS; DIRS +="
> +# convention.
> +DIRS += $(TEST_DIRS)

I would add these to TOOL_DIRS instead of DIRS. TOOL_DIRS get built after the libs phase completes.

Also, this should be inside an ifdef ENABLE_TESTS block, so that Makefiles can simply assign directly to TEST_DIRS without having to have the ifdef.
Comment 2 Gregory Szorc [:gps] 2011-11-11 12:46:50 PST
Thanks, Ted. Will make the changes and resubmit.

Also, how do you feel about defining additional variables, like XPCSHELL_DIRS, MOCHITEST_DIRS, etc? The advantages over vanilla TEST_DIRS is a 'make tests' target (bug 697894) could be implemented easier. I also like being more explicit rather than generic.

If we go with per-test-type variables, I have no clue what the exhaustive list would be...
Comment 3 Gregory Szorc [:gps] 2011-11-11 13:08:15 PST
Created attachment 573900 [details] [diff] [review]
Roll TEST_DIRS into DIRS variable, v2

Moved variable appending to inside "ifdef ENABLE_TESTS".

Also, retained DIRS instead of TOOL_DIRS because this would mean a simple `make -C directory` wouldn't pick up test file changes. See the new comment in the patch. A "proper" implementation can be addressed sometime in the future.
Comment 4 Ted Mielczarek [:ted.mielczarek] 2011-11-11 17:10:08 PST
Comment on attachment 573900 [details] [diff] [review]
Roll TEST_DIRS into DIRS variable, v2

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

This looks good. I'm not sure about test-specific dirs, lots of directories stick all their tests in one dir.
Comment 5 Gregory Szorc [:gps] 2011-11-11 17:31:13 PST
Ah, didn't realize there was a lot of sharing. I guess we can cross that bridge later if we want to.
Comment 6 Ted Mielczarek [:ted.mielczarek] 2011-11-12 06:02:10 PST
I forgot to mention, but make sure you make the same changes to js/src/config/rules.mk before you land this. (Just copy the file over to make them identical.)
Comment 7 Gregory Szorc [:gps] 2011-11-14 11:27:44 PST
pushed to b-s: https://hg.mozilla.org/projects/build-system/rev/5dfafed54b5c

I'd update the docs, but MDN isn't behaving well at the moment.
Comment 9 Atul Aggarwal 2011-11-18 07:49:45 PST
Is the change checked-in in m-c branch so that bug 702388 can be worked on?
Comment 10 Gregory Szorc [:gps] 2011-11-18 11:34:19 PST
It is checked in to m-c now: https://hg.mozilla.org/mozilla-central/rev/5dfafed54b5c

Note You need to log in before you can comment on or make changes to this bug.