772981 - Microformats mochitests Makefile may have been overlooked in bug 370750

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Daniel Holbert [:dholbert]]

The microformats mochitest Makefile adds its mochitests by setting _TEST_FILES, as was the standard practice up until bug 370750 near-globally renamed _TEST_FILES to MOCHITEST_FILES.

Somehow, the microformats Makefile was missed, as it still says _TEST_FILES:
  https://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/Makefile.in#22

That variable almost certainly wants to be renamed to MOCHITEST_FILES.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> https://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/
> Makefile.in#22

er, pasted the wrong URL there (to a different makefile which may or may not have also been missed -- I'm not sure if makefiles in /testing/mochitest/ were intentionally skipped)

The microformats one is:
 https://mxr.mozilla.org/mozilla-central/source/toolkit/components/microformats/tests/Makefile.in#14

Comment 2

[Daniel Holbert [:dholbert]]

I suspect this needs the other tweaks from  bug 370750, too -- rules.mk moving, the "libs" lines being removed, etc.

froydnj, do you know if there's a reason this makefile was skipped?

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: patch

(In reply to Daniel Holbert [:dholbert] from comment #2)
> I suspect this needs the other tweaks from  bug 370750, too -- rules.mk
> moving, the "libs" lines being removed, etc.
> 
> froydnj, do you know if there's a reason this makefile was skipped?

My automated rewriting was looking for mochitests/tests/$(relativesrcdir) or similar, which this Makefile doesn't use, and my eyeballing of libs:: matches to catch stragglers overlooked this one.  Sorry about that!

Comment 4

[Daniel Holbert [:dholbert]]

No worries, thanks for fixing!

Some other candidate Makefiles:
http://mxr.mozilla.org/mozilla-central/search?string=^_TEST_FILES&regexp=1&find=Makefile.in&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

(The two 'identity' Makefiles in that list were just added yesterday in bug 753239, and it looks like they need fixing -- I posted over on that bug.)

Comment 5

[Daniel Holbert [:dholbert]]

Would it be possible/prudent to fail (tests or build) if a mochitest Makefile uses the old format?

I think we need either that, or repeated manual checking over the next few months, to be sure that no one accidentally lands patches with the old format (which they may have started working on back when the old format was still good -- as was the case in bug 753239).

Comment 6

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ed247b1725f

(In reply to Daniel Holbert [:dholbert] from comment #5)
> Would it be possible/prudent to fail (tests or build) if a mochitest
> Makefile uses the old format?
> 
> I think we need either that, or repeated manual checking over the next few
> months, to be sure that no one accidentally lands patches with the old
> format (which they may have started working on back when the old format was
> still good -- as was the case in bug 753239).

Complaining loudly would be the best option, but I don't think it's feasible; _TEST_FILES and friends tend to be defined after rules.mk, so there's no central place to yell about _TEST_FILES &co.  Maybe a special libs:: rule that checks for _TEST_FILES &co. and yells?  Not sure if that would work...

I've already patched a few places that got missed in bug 370750; I'm happy to keep an eye out over the next month or two.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5ed247b1725f