93213 - embedding/browser/gtk/tests built even with --disable-tests

Status: RESOLVED WORKSFORME

(Core Graveyard :: Embedding: GTK Widget, defect)

Description

[Colin Blake]

I build with --disable-tests but embedding/browser/gtk/tests still gets built.
Can/should this get turned off if we're configured with --disable-tests?

Comment 1

[Christopher Blizzard (:blizzard)]

Probably.  There's probably just a test missing around the directory in the
makefile.

Comment 2

[Christopher Blizzard (:blizzard)]

Attachment: patch

Comment 3

[Christopher Blizzard (:blizzard)]

We are going to build this even when tests are disabled at leaf's request.  They
need it for smoketesting.

Comment 4

[Colin Blake]

If a test is needed for a testing purpose, shouldn't that build be done WITHOUT
--disable-tests?

Comment 5

[Chris McAfee]

we switched from gtkEmbed to TestGtkEmbed as our default test
embed app, I wanted this built by default so we don't
regress there. I believe that is why we changed the tests directory
to be built by default.  Perhaps we could rearrange directories
to satisfy everyone?  Adding leaf who r='d this change earlier
this summer.

Comment 6

[Daniel (Leaf) Nunes]

colin, the problem is that we have different levels of testing; we don't want to
have to build *every* test in the release builds (increasing build time/package
size) just to get one or two really high level testing applications.

mcafee, changing directories is probably a good idea, but the objection is still
the same... it's a testing thing that a lot of people don't care about.

Really, we should break down the kinds of tests that we can build, and have
flags that can override the --disable-tests option in order to build specific
test apps (--enable-embedding-tests ?).

Comment 7

[hacker formerly known as [e-mail]]

*** Bug 128583 has been marked as a duplicate of this bug. ***

Comment 8

[Ben Bucksch (:BenB)]

> --enable-embedding-tests

Yes, I think this is better.

If I say "no tests", then give me no tests, please :-). REOPENing.

This might be important, if a distributor doesn't use the packages-<platform>
files, but just zipps up dist/bin. TestGtkEmbed shoudn't ship to users.

(Beonex currently does use packages-<platform> files, so we are not affected by
this bug other than during testing.)

Comment 9

[Mike Hommey [:glandium]]

Attachment: new patch

This patch implements the proposed behaviour.

Comment 10

[Benjamin Smedberg]

Comment on attachment 193727 [details] [diff] [review]
new patch

You don't need to include configure in the patch... our CVS system auto-commits
configure whenever configure.in is changed.

in embedding/browser/gtk/Makefile.in you don't need all the DO_TESTS stuff: if
you really need to check ENABLE_TESTS *and* ENABLE_EMBEDDING_TESTS you can do

ifneq (,$(ENABLE_TESTS)$(ENABLE_EMBEDDING_TESTS))

but I think that you can safely just do

ifdef ENABLE_EMBEDDING_TESTS here, right? There are situations where people
might want to --enable-tests --disable-embedding-tests

Comment 11

[Mike Hommey [:glandium]]

Attachment: New patch

You're right. Here is the modified patch.

Comment 12

[Benjamin Smedberg]

Comment on attachment 193933 [details] [diff] [review]
New patch

You don't need CPPSRCS = $(NULL), just remove that line altogether....

actually, why do you need the change in embedding/browser/gtk/tests/Makefile.in
at all? We're not going to build that directory unless ENABLE_EMBEDDING_TESTS
is set, right?

Comment 13

[Benjamin Smedberg]

Comment on attachment 193933 [details] [diff] [review]
New patch

clearing review based on the question about
embedding/browser/gtk/tests/Makefile.in -- please re-request review if this is
somehow correct.

Comment 14

[Mike Hommey [:glandium]]

Attachment: Updated patch

You're right. Simplified patch attached.

Comment 15

[Mike Hommey [:glandium]]

could this be landed somehow ?

Comment 16

[Reed Loden [:reed]]

Mike, if you wouldn't mind making sure this patch still works against CVS, I can help you get it checked into the tree (once it has approval).

Comment 17

[Mike Hommey [:glandium]]

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=Makefile.in&branch=1.13&root=/cvsroot&subdir=mozilla/embedding/browser/gtk&command=DIFF_FRAMESET&rev1=1.8&rev2=1.9

This change made the test not building when disabling tests, but there is no bug reference to know why this has been done, especially not even notifying the current bug, considering concerns had been raised about necessity of the test.

The strange thing is that embedding/browser/gtk/gtkembed.pkg has not been changed, I don't know if it implies some breakage when --disable-tests is used (not tested).

So now the question is, is the --enable-embedding-tests still needed ?

Comment 18

[Reed Loden [:reed]]

bsmedberg/ted, think one of you could answer Mike's question in comment #17?

Comment 19

[Mike Hommey [:glandium]]

Let's just assume the answer to comment #17 is "no".