487559 - Make test only if --enable-tests option

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; ja; rv:1.8.0.4) Gecko/20060508 Firefox/3.0
Build Identifier: 

Make test only if --enable-tests option in toolkit/mozapps/downloads/.

Reproducible: Always

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: trivial patch

Comment 2

[Shawn Wilsher :sdwilsh]

Comment on attachment 371804 [details] [diff] [review]
trivial patch

I'm not even sure if this is needed, but ted should take a look.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Remove a redundant endif.

I am sorry, the previous patch has a redundant endif.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Update patch [Checkin: Comment 10 & 11]

I missed again. Fixed line number.

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 372002 [details] [diff] [review]
Update patch
[Checkin: Comment 10 & 11]

>+DIRS += tests

Even |PARALLEL_DIRS += tests|?

And the file in 'tests' dir needs the same fix(es), for consistency.

Comment 6

[(not currently active) Ted Mielczarek]

(In reply to comment #5)
> Even |PARALLEL_DIRS += tests|?

What? That doesn't make any sense. There's no use of || dirs here.

> And the file in 'tests' dir needs the same fix(es), for consistency.

Why? The tests dir will never be entered if tests are disabled, so it doesn't matter what that makefile does.

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)

> What? That doesn't make any sense. There's no use of || dirs here.

Would it hurt? Would be useful if there are other PARALLEL_DIRS sometime, no?

> Why? The tests dir will never be entered if tests are disabled, so it doesn't
> matter what that makefile does.

What if someone tries to run the tests when already being in the subdir?

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 372002 [details] [diff] [review]
Update patch
[Checkin: Comment 10 & 11]

>+DIRS = src

Nit: Maybe add a blank line here.

>+ifdef ENABLE_TESTS
>+DIRS += tests
>+endif
> endif
> 
> include $(topsrcdir)/config/rules.mk
> 
> 

Nit: Maybe remove the 2 ending blank lines.

*****

(In reply to comment #7)
> What if someone tries to run the tests when already being in the subdir?

s/the tests/some test commands/

Comment 9

[(not currently active) Ted Mielczarek]

This patch is fine, it doesn't need any changes. If you make in a test subdirectory with tests disabled, well, it's your own fault.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/ed499003e577

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 372002 [details] [diff] [review]
Update patch
[Checkin: Comment 10 & 11]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/61643ceaaf7b

Comment 12

[Aakash Desai [:aakashd]]

Is there a way to verify this bug by checking to see if the tests have been run? Right now, verification for this bug would be to just look at the makefile in the directory and verify the code is there. 


If so, I can go ahead and verify it, but would like to find a better way to test this.

Comment 13

[(not currently active) Ted Mielczarek]

We don't generally worry about testing/verifying simple makefile fixes like this.

Comment 14

[Aakash Desai [:aakashd]]

(In reply to comment #13)
> We don't generally worry about testing/verifying simple makefile fixes like
> this.

You might not, but I would certainly like to verify any or all bugs that can be verifiable. So, is there a way?

Comment 15

[Shawn Wilsher :sdwilsh]

verify by code inspection?

Comment 16

[Aakash Desai [:aakashd]]

okey dokey, if that's the only way to verify this, then verified FIXED on debug builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090602 Shiretoko/3.5pre ID:20090602031310

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090528 Minefield/3.6a1pre ID:20090528112613