The default bug view has changed. See this FAQ.

Add TEST_DIRS Variable

RESOLVED FIXED in mozilla11

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

Trunk
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
Attachment #573894 - Flags: review?(ted.mielczarek)
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.
Attachment #573894 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Comment 2

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

Comment 3

5 years ago
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.
Assignee: nobody → gps
Attachment #573894 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #573900 - Flags: review?(ted.mielczarek)
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.
Attachment #573900 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 5

5 years ago
Ah, didn't realize there was a lot of sharing. I guess we can cross that bridge later if we want to.
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.)
(Assignee)

Comment 7

5 years ago
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.
Keywords: dev-doc-needed
(Assignee)

Updated

5 years ago
Blocks: 702388
(Assignee)

Comment 8

5 years ago
Added docs to https://developer.mozilla.org/index.php?title=en/How_Mozilla%27s_build_system_works/Makefile_-_variables
Keywords: dev-doc-needed

Comment 9

5 years ago
Is the change checked-in in m-c branch so that bug 702388 can be worked on?
(Assignee)

Comment 10

5 years ago
It is checked in to m-c now: https://hg.mozilla.org/mozilla-central/rev/5dfafed54b5c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Blocks: 735433

Updated

5 years ago
Blocks: 759013
You need to log in before you can comment on or make changes to this bug.