Status

defect
--
trivial
RESOLVED INCOMPLETE
11 years ago
2 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.2a1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed1.9.1b99: Av2a])

Attachments

(3 attachments, 2 obsolete attachments)

Nits or fixes found while checking bug 485736.
Flags: in-testsuite-
Depends on: 370407
Passed on Tryserver as 'try-9534e0d4ded'.
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #372585 - Flags: review?(ted.mielczarek)
Attachment #372587 - Flags: review? → review?(philipp)
Blocks: 462381
Comment on attachment 372587 [details] [diff] [review]
(Bv1-CC) </calendar/>: Remove empty targets, use PARRALEL_DIRS
[Checkin: See comment 5]

r=philipp
Attachment #372587 - Flags: review?(philipp) → review+
Attachment #372585 - Flags: review?(ted.mielczarek) → review-
Comment on attachment 372585 [details] [diff] [review]
(Av1) Remove empty targets, add ifdef, use PARRALEL_DIRS

What's the point of using PARALLEL_DIRS here? It doesn't give any advantage unless you have more than one directory in it.
Comment on attachment 372587 [details] [diff] [review]
(Bv1-CC) </calendar/>: Remove empty targets, use PARRALEL_DIRS
[Checkin: See comment 5]


http://hg.mozilla.org/comm-central/rev/c8cdbc3a283c
Bv1-CC, target removal only.
Attachment #372587 - Attachment description: (Bv1-CC) </calendar/>: Remove empty targets, use PARRALEL_DIRS → (Bv1-CC) </calendar/>: Remove empty targets, use PARRALEL_DIRS [Checkin: See comment 5]
Av1, with comment 4 suggestion(s).
Attachment #372585 - Attachment is obsolete: true
Attachment #374837 - Flags: review?(ted.mielczarek)
I tried this patch on Try server and the tests are not executed anymore.
Then I wonder about
http://hg.mozilla.org/mozilla-central/diff/70dd0e9f14c2/security/manager/Makefile.in
(This seems to be the only place where ENABLE_TESTS is used in nsprpub+nss+security...)

Honza, do your ENABLE_TESTS actually work?
What would I be doing wrong?
Attachment #374911 - Flags: review?(wtc)
Attachment #374911 - Flags: review?(honzab.moz)
Comment on attachment 374911 [details] [diff] [review]
(Cv1) </security/>: Remove empty targets, add ifdef

Serge, thanks for the patch.

Your change to security/manager/ssl/Makefile.in is good.

I believe that your changes to security/manager/ssl/tests/Makefile.in
and security/manager/ssl/tests/mochitest/Makefile.in are
not necessary because we only recurse into security/manager/ssl/tests
if ENABLE_TESTS is defined, due to your change to
security/manager/ssl/Makefile.in.


I would not modify security/nss/tests/pkcs11/netscape/trivial/Makefile.in
because it is obsolete code.  If you really want to remove the no-op
targets, please also update this comment:

> # Now, various standard targets, some that do stuff, some that are no-ops
Attachment #374911 - Flags: review?(wtc) → review-
Comment on attachment 374837 [details] [diff] [review]
(Av2) Remove empty targets, add ifdef
[Checkin: See comment 10 & 11]

--- a/modules/libpr0n/test/Makefile.in
+ifdef ENABLE_TESTS
 DIRS            += mochitest
+endif

Leave this out, this test directory is already ifdef ENABLE_TESTS in the parent Makefile.
Attachment #374837 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 374837 [details] [diff] [review]
(Av2) Remove empty targets, add ifdef
[Checkin: See comment 10 & 11]


http://hg.mozilla.org/mozilla-central/rev/20ab96ddb645
Av2, with comment 9 suggestion(s).
Attachment #374837 - Attachment description: (Av2) Remove empty targets, add ifdef → (Av2) Remove empty targets, add ifdef [Checkin: See comment 10]
Comment on attachment 374837 [details] [diff] [review]
(Av2) Remove empty targets, add ifdef
[Checkin: See comment 10 & 11]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/90222479919c
Attachment #374837 - Attachment description: (Av2) Remove empty targets, add ifdef [Checkin: See comment 10] → (Av2) Remove empty targets, add ifdef [Checkin: See comment 10 & 11]
Keywords: fixed1.9.1
Whiteboard: [fixed1.9.1b5: Av2a]
Target Milestone: --- → mozilla1.9.2a1
Attachment #374911 - Flags: review?(honzab.moz)
Comment on attachment 374911 [details] [diff] [review]
(Cv1) </security/>: Remove empty targets, add ifdef

If I understand correctly what the patch has to do I can say I have same objections as Wan-Teh in comment 8. 

(In reply to comment #7)
> Then I wonder about
> http://hg.mozilla.org/mozilla-central/diff/70dd0e9f14c2/security/manager/Makefile.in
> (This seems to be the only place where ENABLE_TESTS is used in
> nsprpub+nss+security...)
> 
> Honza, do your ENABLE_TESTS actually work?
> What would I be doing wrong?

This hunk compiles some binaries (utils to manipulate certificates) needed to have SSL support in mochitest. It works, AFAIK.

I don't know what has been going wrong on the tryserver, could you refer the log or did you try the patch locally? If you wish I can do it my self if you more describe what is wrong in detail.

I'm not that familiar with what you are trying to achieve: 'make check' have to invoke all tests (xpc, ref, mochi, chrome, browser...) and therefor you made a new target to run xpc-tests?
Cv1, with comment 8 suggestion(s),
and some more.

I could remove the 'check' shortcut if you prefer,
otherwise here is the new/fixed 'xpcshell-tests' shortcut.

|+include $(DEPTH)/config/autoconf.mk|
That's why ENABLE_TESTS was not defined in that file!
I guess it's all right to include it here?
Do you want to include it in the ('tests') sub-directories too, just to be future-proof?


(In reply to comment #8)
> I would not modify security/nss/tests/pkcs11/netscape/trivial/Makefile.in
> because it is obsolete code.

It does looks like so: would it be possible to rather remove some of these files?


(In reply to comment #12)
> This hunk compiles some binaries (utils to manipulate certificates) needed to
> have SSL support in mochitest. It works, AFAIK.

Indeed, I verified that :-)

> I don't know what has been going wrong on the tryserver

I eventually found out, thanks for having looked at it, Honza.
Attachment #374911 - Attachment is obsolete: true
Attachment #375630 - Flags: review?(wtc)
Comment on attachment 375630 [details] [diff] [review]
(Cv2) </security/>: update shortcut, add ifdef, remove empty targets

r=wtc.

1. I don't understand your change to security/manager/Makefile.in.

2. In security/manager/ssl/Makefile.in, I would try to align the
= and += with the = signs above.

3. Please exclude security/nss/tests/pkcs11/netscape/trivial/Makefile.in
from this patch.  security/nss/tests/pkcs11 is obsolete code that
hope will be resurrected some day.  Until then let's not fix cosmetic
problems in it because we can't test the patch.
Attachment #375630 - Flags: review?(wtc) → review+
(In reply to comment #14)

> 1. I don't understand your change to security/manager/Makefile.in.

Bug 485736 split 'check' target into 'xpcshell-tests';
'ssl' directory doesn't seem to execute anything, the tests are in 'ssl/tests' directory actually.
Whiteboard: [fixed1.9.1b5: Av2a] → [fixed1.9.1b99: Av2a]
`make check` is still pretty bad to this day. But I don't see much value in keeping this bug open.
Status: ASSIGNED → RESOLVED
Closed: 2 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.