Last Comment Bug 722075 - testing/mochitest/tests/browser/ has commented-out lines before $(NULL)
: testing/mochitest/tests/browser/ has commented-out lines before $(...
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
-- normal (vote)
: Firefox 12
Assigned To: Daniel Holbert [:dholbert]
Depends on:
  Show dependency treegraph
Reported: 2012-01-28 11:52 PST by Daniel Holbert [:dholbert]
Modified: 2012-01-31 07:04 PST (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

fix (1.37 KB, patch)
2012-01-28 11:53 PST, Daniel Holbert [:dholbert] review+
Details | Diff | Splinter Review

Description User image Daniel Holbert [:dholbert] 2012-01-28 11:52:13 PST
The file testing/mochitest/tests/browser/ has some commented-out lines at the end of its test lists, before the final $(NULL)

IIRC, any commented-out lines in the middle of a Makefile's list of mochitests will end the list prematurely, right at that point -- so really we want the list-terminating $(NULL) to be *before* the commented-out lines.

(In this case it doesn't really matter because there aren't any actual tests after the commented-out chunk; still, we might as well fix this in case any tests are added/uncommented later on, and for style/consistency/correctness points).
Comment 1 User image Daniel Holbert [:dholbert] 2012-01-28 11:53:54 PST
Created attachment 592431 [details] [diff] [review]

Tagging gavin for r?, since his name's at the top of the file.
Comment 2 User image :Gavin Sharp [email:] 2012-01-29 17:22:11 PST
Comment on attachment 592431 [details] [diff] [review]

I left it this way because it makes it easier to re-enable those tests (you don't need to move the $(NULL) around), and AFAICT there is no practical impact. I guess people don't do that enough for it to matter.
Comment 3 User image Daniel Holbert [:dholbert] 2012-01-30 12:15:43 PST
> AFAICT there is no practical impact

None right now, correct -- but if someone were to add some more tests between the commented-out chunk and the $(NULL) line, e.g. to maintain alphabetical ordering or consistency or something, then it'd be bad - those added tests would never be run.  So I think best-practice is to have $(NULL) before the commented-out lines, to make it clear where the list actually ends.

Thanks for the review! Landed:
Comment 4 User image Ed Morley [:emorley] 2012-01-31 07:04:28 PST

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