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

RESOLVED WORKSFORME

Status

Core Graveyard
Embedding: GTK Widget
RESOLVED WORKSFORME
17 years ago
6 years ago

People

(Reporter: Colin Blake, Assigned: glandium)

Tracking

Trunk
x86
Windows 98

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

17 years ago
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?
Probably.  There's probably just a test missing around the directory in the
makefile.
Created attachment 44810 [details] [diff] [review]
patch
Whiteboard: have patch; needs r=
We are going to build this even when tests are disabled at leaf's request.  They
need it for smoketesting.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → WONTFIX
(Reporter)

Comment 4

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

Comment 5

16 years ago
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

16 years ago
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 ?).
*** Bug 128583 has been marked as a duplicate of this bug. ***

Comment 8

16 years ago
> --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.)
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Assignee)

Comment 9

13 years ago
Created attachment 193727 [details] [diff] [review]
new patch

This patch implements the proposed behaviour.
Attachment #193727 - Flags: review?(benjamin)

Comment 10

13 years ago
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
Attachment #193727 - Flags: review?(benjamin) → review-
(Assignee)

Comment 11

13 years ago
Created attachment 193933 [details] [diff] [review]
New patch

You're right. Here is the modified patch.
Attachment #193727 - Attachment is obsolete: true
Attachment #193933 - Flags: review?(benjamin)

Comment 12

13 years ago
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

13 years ago
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.
Attachment #193933 - Flags: review?(benjamin)
(Assignee)

Comment 14

13 years ago
Created attachment 194287 [details] [diff] [review]
Updated patch

You're right. Simplified patch attached.
Attachment #193933 - Attachment is obsolete: true
Attachment #194287 - Flags: review?(benjamin)

Updated

13 years ago
Attachment #194287 - Flags: review?(benjamin) → review+
(Assignee)

Comment 15

12 years ago
could this be landed somehow ?

Updated

11 years ago
Attachment #194287 - Flags: superreview?(benjamin)

Updated

11 years ago
Attachment #194287 - Flags: superreview?(benjamin) → superreview+
Assignee: blizzard → nobody
Status: REOPENED → NEW
QA Contact: pavlov → gtk-widget
Assignee: nobody → mh+mozilla
Whiteboard: have patch; needs r=
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).
(Assignee)

Comment 17

10 years ago
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 ?
bsmedberg/ted, think one of you could answer Mike's question in comment #17?
(Assignee)

Comment 19

8 years ago
Let's just assume the answer to comment #17 is "no".
Status: NEW → RESOLVED
Last Resolved: 16 years ago8 years ago
Resolution: --- → WORKSFORME
Component: Embedding: GTK Widget → Embedding: GTK Widget
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.