988532 - Remove jit-tests from 'make check'

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Dan Minor [:dminor]]

Once the jit-tests are scheduled to run from the test package in Bug 973900 and working satisfactorily, we can remove them from make check.

Comment 1

[Dan Minor [:dminor]]

RyanVM, I remember you mentioned in irc wanting to leave jit-tests in make check for some build types. Is that still the case, and which builds would you want this done for?

Easy enough to add a 'make jittest' target, but we'd have to sort out how to handle the buildbot config side of things.

Thanks!

Comment 2

[Ryan VanderMeulen [:RyanVM]]

- ASAN debug (checktests are the only test coverage we've got on those builds)
- Spidermonkey shell builds
- *Maybe* B2G desktop builds
  * I don't have a horribly strong opinion on this one and suspect we could probably live without it like we already are for Cpp unit tests, but I'm also not sure the loss of Cpp unit test coverage was on purpose.

Comment 3

[Dan Minor [:dminor]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #2)
> - ASAN debug (checktests are the only test coverage we've got on those
> builds)
> - Spidermonkey shell builds
> - *Maybe* B2G desktop builds
>   * I don't have a horribly strong opinion on this one and suspect we could
> probably live without it like we already are for Cpp unit tests, but I'm
> also not sure the loss of Cpp unit test coverage was on purpose.

The loss of Cpp unit test coverage was accidental. If you can live without this (at least for the time being) this sounds like good fodder for a follow up bug. Thanks!

Comment 5

[Dan Minor [:dminor]]

Attachment: bug-988532-fix.patch

Try run here: https://tbpl.mozilla.org/?tree=Try&rev=f69ccc8f2eaf

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 8401790 [details] [diff] [review]
bug-988532-fix.patch

Review of attachment 8401790 [details] [diff] [review]:
-----------------------------------------------------------------

This is a pretty big break from convention. Anyone would expect `make check` to run the tests.

Spidermonkey devs don't use `make check`, but it's literally one of the first things a new volunteer contributor is likely to try, and can we please make it not be some crazy custom Mozilla thing that they find out later was not what they wanted at all?

Maybe the build could run a different target, `make quick-check` or `make smoketest`, instead? Then

    check:: quick-check check-jit-test check-jstests

This way we could also re-add check-jstests to `make check`, and tinderbox wouldn't care. \o/

Comment 7

[Jonathan Griffin (:jgriffin)]

Ted, can you weigh in here?

Comment 8

[(dormant account)]

I think something got lost in translation. My objective with removing jittests is to make our infra build faster. Rest of the stuff in make check is less of a time-sink, but it's still stuff that should not be run by default on builders.
The goal here is to not run 'make check', not to make 'make check' run less tests. Though I'm ok with something less pretty as an intermediate step towards a faster building world.

Comment 9

[Dan Minor [:dminor]]

Almost none of the test suites that need to be run prior to landing a patch are currently part of "make check", so I don't see this as a big break from convention. New contributors already have to learn how to run the the test suites that are not part of "make check".

Comment 10

[Phil Ringnalda (:philor)]

Couple of other things to keep in mind while considering the sanctity of make check - we already removed the compiled code tests from it, and, it's a totally random agglomeration of a tiny percentage of our tests already. make check includes tests for the toolkit feed parser, and tests of the xpcshell harness but not the things the xpcshell harness tests, and tests for a harness that does some kind of something on devices or maybe only on emulators of devices, I've never been quite sure what DeviceManager actually does. The very best thing we could do for comment 6's concerns about expectations would be to have make check do nothing but output the URL for the devmo page on test suites and how to run them.

Comment 11

[(not currently active) Ted Mielczarek]

I think the win from getting these tests out of "make check" in the short term outweighs the usefulness of having them in there for contributors. We have a lot of tests, expecting "make check" to tell you everything is not reasonable.

Longer-term we would like to probably stop running "make check" directly on the build farm, and simply run the small set of build system checks directly, but that's not going to happen overnight.

(In reply to Jason Orendorff [:jorendorff] from comment #6)
> This is a pretty big break from convention. Anyone would expect `make check`
> to run the tests.
> 
> Spidermonkey devs don't use `make check`,

This argument doesn't convince me much. "We don't use it. But people might!"

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8401790 [details] [diff] [review]
bug-988532-fix.patch

Review of attachment 8401790 [details] [diff] [review]:
-----------------------------------------------------------------

I empathize with jorendorff that decapitating |make check| may surprise some. However, IMO the greater good is moving these tests off of build slaves in automation. Furthermore, |make check| doesn't run every test under the sun for the rest of Firefox. We've learned to cope. SpiderMonkey developers can too.

If we really care, I think it would be reasonable to have an |ifndef IS_AUTOMATION check:: check-jit-test| conditional block so local developers can still do everything via |make check| without the harm on automation. We can do that here or as a follow-up. (BTW, "IS_AUTOMATION" isn't a real variable - I can't remember what the real one is.)

Comment 13

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #12)
> (BTW, "IS_AUTOMATION" isn't a real variable - I can't remember what the real one is.)

The closest we have to that currently is TINDERBOX_OUTPUT.

Comment 14

[Jason Orendorff [:jorendorff]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #11)
> I think the win from getting these tests out of "make check" in the short
> term outweighs the usefulness of having them in there for contributors.

Just how short-term do we have to think here? I thought I proposed something concrete that should meet everyone's needs and would be maybe a 12-line patch instead of a 1-line patch.

> [...] We
> have a lot of tests, expecting "make check" to tell you everything is not
> reasonable.

Well — first of all, it's completely reasonable inside js/src.

But OK. I would also be very happy if "make check" replied with a short help message explaining that the tests are divided into several targets, and what they are.

Either way is fine.

I get that the usual Unix conventions can't apply to us, not everywhere. But if we get in the habit of breaking them gratuitously, we'll be continually eroding the usability of the codebase.

If I understood better why this is so hard, I wouldn't be making a fuss. It seems like a five-minute hack.

Comment 15

[Jason Orendorff [:jorendorff]]

> If I understood better why this is so hard, I wouldn't be making a fuss. It
> seems like a five-minute hack.

Oh, is it because of the way the `make check` machinery in rules.mk uses LOOP_OVER_DIRS, and we don't want to complicate it for the sake of standalone spidermonkey?

Bah. Well, OK then.

Comment 16

[Dan Minor [:dminor]]

Thanks, pushed to: https://hg.mozilla.org/integration/mozilla-inbound/rev/b9ca131099c4

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b9ca131099c4