testing/mochitest/tests/browser/Makefile.in has commented-out lines before $(NULL)

RESOLVED FIXED in Firefox 12

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
Firefox 12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

The file testing/mochitest/tests/browser/Makefile.in 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).
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
Created attachment 592431 [details] [diff] [review]
fix

Tagging gavin for r?, since his name's at the top of the file.
Attachment #592431 - Flags: review?
Attachment #592431 - Flags: review? → review?(gavin.sharp)
Comment on attachment 592431 [details] [diff] [review]
fix

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.
Attachment #592431 - Flags: review?(gavin.sharp) → review+
> 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:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/3c2a8aa90e06
Target Milestone: --- → Firefox 12
https://hg.mozilla.org/mozilla-central/rev/3c2a8aa90e06
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.