701822 - Add TEST_DIRS Variable

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

Attachment: 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

[(not currently active) 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.

Comment 2

[Gregory Szorc [:gps]]

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]]

Attachment: 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

[(not currently active) 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.

Comment 5

[Gregory Szorc [:gps]]

Ah, didn't realize there was a lot of sharing. I guess we can cross that bridge later if we want to.

Comment 6

[(not currently active) Ted Mielczarek]

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]]

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 8

[Gregory Szorc [:gps]]

Added docs to https://developer.mozilla.org/index.php?title=en/How_Mozilla%27s_build_system_works/Makefile_-_variables

Comment 9

[Atul Aggarwal]

Is the change checked-in in m-c branch so that bug 702388 can be worked on?

Comment 10

[Gregory Szorc [:gps]]

It is checked in to m-c now: https://hg.mozilla.org/mozilla-central/rev/5dfafed54b5c