Closed Bug 487559 Opened 13 years ago Closed 13 years ago

Make test only if --enable-tests option

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
trivial

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: hiro, Assigned: hiro)

Details

(Keywords: verified1.9.1, Whiteboard: [fixed1.9.1b99])

Attachments

(1 file, 2 obsolete files)

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
Attached patch trivial patch (obsolete) — Splinter Review
Comment on attachment 371804 [details] [diff] [review]
trivial patch

I'm not even sure if this is needed, but ted should take a look.
Attachment #371804 - Flags: review?(ted.mielczarek)
Attached patch Remove a redundant endif. (obsolete) — Splinter Review
I am sorry, the previous patch has a redundant endif.
Attachment #371804 - Attachment is obsolete: true
Attachment #371959 - Flags: review?(ted.mielczarek)
Attachment #371804 - Flags: review?(ted.mielczarek)
I missed again. Fixed line number.
Attachment #371959 - Attachment is obsolete: true
Attachment #372002 - Flags: review?(ted.mielczarek)
Attachment #371959 - Flags: review?(ted.mielczarek)
Attachment #372002 - Flags: review?(ted.mielczarek) → review+
Assignee: nobody → ikezoe
Keywords: checkin-needed
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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: in-testsuite-
Version: unspecified → Trunk
(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.
(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 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/
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.
http://hg.mozilla.org/mozilla-central/rev/ed499003e577
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Severity: normal → trivial
Keywords: checkin-needed
Whiteboard: [needs 1.9.1 landing]
Target Milestone: --- → mozilla1.9.2a1
Comment on attachment 372002 [details] [diff] [review]
Update patch
[Checkin: Comment 10 & 11]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/61643ceaaf7b
Attachment #372002 - Attachment description: Update patch → Update patch [Checkin: Comment 10 & 11]
Whiteboard: [needs 1.9.1 landing] → [fixed1.9.1b5]
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.
We don't generally worry about testing/verifying simple makefile fixes like this.
(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?
verify by code inspection?
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
Status: RESOLVED → VERIFIED
Whiteboard: [fixed1.9.1b5] → [fixed1.9.1b99]
You need to log in before you can comment on or make changes to this bug.