Closed Bug 370407 Opened 17 years ago Closed 7 years ago

if you do "make check" and you didn't do --enable-tests, print out a nice error message

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: moco, Unassigned)

References

Details

Attachments

(1 file)

if you do "make check" and you didn't do --enable-tests, print out a nice error message.

jminta just ran into this, and I did as well (back a while ago).

it should be easy to fix with some makefile.  

perhaps it shouldn't just be in places, though.
could we force the creation of a place-holder file when configured and built with --enable-tests and then check for that file's existence in make check?
I was thinking of using ENABLE_TESTS inside the Makefile, so that if not set, and you do check target, we fail and report an error.

that sounds easier. Let's do it your way.
Then we should alert when the target is other than |tests| folder?

For example, instead of
make -C $(MOZ_OBJDIR)/browser/components/places/tests check
,
make -C $(MOZ_OBJDIR)/browser/components/places check
this hack does nothing. Just keeping quiet.

On the other hand, if --enable-tests is set,
make -C $(MOZ_OBJDIR)/browser/components/places check
or
make -C $(MOZ_OBJDIR) check
should start testing as expected, regardless of this patch.
Component: Places → Testing
Product: Firefox → Core
Attachment #255285 - Flags: review?(rcampbell)
An echo doesn't abort recursion immediately, which doesn't seem right (imagine getting hundreds of echos from |make -C objdir/ check|).  Perhaps change that to
$(error Tests are disabled. Please set --enable-tests.) instead?
Attachment #255285 - Flags: review?(rcampbell)
QA Contact: places → testing
Comment on attachment 255285 [details] [diff] [review]
proposal: rules.mk

as per jeff's comments re: recursion. See recursion.
Attachment #255285 - Flags: review-
Assignee: nobody → ispiked
Is this really solving the problem in the right place?  What I mean is, if we want to get the largest number of developers running tests, we should make that as easy as possible: 'make check' should Just Work in a default build.  Maybe that means --enable-tests on by default, or maybe it should not be guarded by a configure option at all.
While it would be nice to enable tests by default, currently enabling tests pollutes dist/bin with extra test files. We would have to fix that problem before flipping any defaults.
(In reply to comment #8)
> While it would be nice to enable tests by default, currently enabling tests
> pollutes dist/bin with extra test files. We would have to fix that problem
> before flipping any defaults.

This is easy to fix, at least for netwerk/tests. I did not --enable-tests, but on sayrer's advice did cd netwerk/test; make check. That failed, but make followed by make check worked. Isn't this just a missing dependency in the makefile?

/be
It seems to be working for me now, so I'm suspecting that I misunderstood what was going on and had a problem similar to brendan's.
I didn't have a problem, perhaps I just found a missing dependency bug.

To repeat, I never reconfigured with --enable-tests. But make;make check in netwerk tests succeeded, after make check alone failed. Seems to me computers should work for us.

In the future, when apes rule the world, it will all be different, I'm sure ;-). But for now, couldn't we have a dependency to automate what I had to do by hand? Or is --enable-tests more generally required, and netwerk/tests just happens to be simple?

/be
netwerk/tests is simpler than most in that it doesn't have reftests. I don't think that trying to have "make check" know to build the tests that it should have built already during the "libs" phase is a good idea. Besides which running "make" in netwerk/tests is what does the dist/bin pollution that I was avoiding in the first place.
(In reply to comment #12)
> netwerk/tests is simpler than most in that it doesn't have reftests.

Ok.

> I don't
> think that trying to have "make check" know to build the tests that it should
> have built already during the "libs" phase is a good idea.

Why not?

> Besides which
> running "make" in netwerk/tests is what does the dist/bin pollution that I was
> avoiding in the first place.

I'm not arguing that netwerk/tests be "made" during normal builds. Only that if I cd there and type 'make check' it ought not crap out for want of a prior 'make'. Who is in charge, the computers or the apes?

/be
Because the "make check" would end up having side-effects that affects the shipping product, which it currently doesn't. I think that we should maintain an invariant that "make check doesn't affect dist/bin". That said, it would certainly be good to issue appropriate errors and have "make check" fail gracefully if the necessary tests haven't actually been built.
Assignee: ispiked → nobody
This bug might not really matter anymore, seeing as how --enable-tests is the default. Then again, if you build --disable-tests, do we still just blindly recurse through the tree if you "make check"?
Component: Testing → Build Config
QA Contact: testing → build-config
Blocks: 488091
OS: Windows XP → All
Hardware: x86 → All
Mass bug cleanup for Core:Build Config.

If you feel this bug has been closed in error, please re-open with new details.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: