Closed
Bug 520743
Opened 16 years ago
Closed 16 years ago
xpcshell-tests: some Places unit tests fail when 'werror' is on.
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 3.7a1
People
(Reporter: lusian, Assigned: lusian)
References
Details
Attachments
(5 files, 3 obsolete files)
21.52 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
5.27 KB,
patch
|
Details | Diff | Splinter Review | |
1.13 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
1.07 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
696 bytes,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/532.0 (KHTML, like Gecko) Chrome/3.0.195.25 Safari/532.0
Build Identifier:
$ make SOLO_FILE="test_browserGlue_corrupt.js" -C browser/components/places/tes
ts/ check-one
make: Entering directory `/c/mozilla-build/mozilla-central/browser/components/pl
aces/tests'
c:/mozilla-build/python25/python2.5.exe -u /c/mozilla-build/mozilla-central/conf
ig/pythonpath.py \
-I/c/mozilla-build/mozilla-central/build \
/c/mozilla-build/mozilla-central/testing/xpcshell/runxpcshelltests.py
\
--symbols-path=../../../../dist/crashreporter-symbols \
--test-path=test_browserGlue_corrupt.js \
../../../../dist/bin/xpcshell \
../../../../_tests/xpcshell/test_browser_places/unit
TEST-UNEXPECTED-FAIL | c:\mozilla-build\mozilla-central\_tests\xpcshell\test_bro
wser_places\unit\test_browserGlue_corrupt.js | test failed (with xpcshell return
code: 0), see following log:
>>>>>>>
TEST-INFO | (xpcshell/head.js) | test 1 pending
TEST-PASS | c:/mozilla-build/mozilla-central/_tests/xpcshell/test_browser_places
/unit/head_bookmarks.js | [create_bookmarks_html : 184] true == true
TEST-PASS | c:/mozilla-build/mozilla-central/_tests/xpcshell/test_browser_places
/unit/head_bookmarks.js | [create_bookmarks_html : 188] true == true
TEST-PASS | c:/mozilla-build/mozilla-central/_tests/xpcshell/test_browser_places
/unit/head_bookmarks.js | [create_JSON_backup : 233] true == true
TEST-PASS | c:/mozilla-build/mozilla-central/_tests/xpcshell/test_browser_places
/unit/head_bookmarks.js | [create_JSON_backup : 237] true == true
TEST-PASS | c:/mozilla-build/mozilla-central/_tests/xpcshell/test_browser_places
/unit/head_bookmarks.js | [create_JSON_backup : 241] true == true
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | TypeError: assignment to undeclared
variable corruptDB
TEST-PASS | c:/mozilla-build/mozilla-central/_tests/xpcshell/test_browser_places
/unit/tail_bookmarks.js | [null : 64] false == false
<<<<<<<
INFO | Result summary:
INFO | Passed: 0
INFO | Failed: 1
make: *** [check-one] Error 1
make: Leaving directory `/c/mozilla-build/mozilla-central/browser/components/pla
ces/tests'
Reproducible: Always
Steps to Reproduce:
1. Run the xpcshell-test with the '-S' option (turn on 'strict' & 'werror')
2.
3.
Actual Results:
Failed
Expected Results:
Passed
Assignee | ||
Comment 1•16 years ago
|
||
Comment 2•16 years ago
|
||
are those all Places tests that are failing in that mode?
feel free to ask me review on any test patch you file under Places.
Assignee | ||
Comment 3•16 years ago
|
||
I ran these tests:
make -C browser/components/places/tests/ xpcshell-tests
make -C toolkit/components/places/tests/ xpcshell-tests
Are there any other places tests?
Assignee | ||
Updated•16 years ago
|
Attachment #404795 -
Flags: review?(mak77)
Assignee | ||
Updated•16 years ago
|
Attachment #404903 -
Flags: review?(mak77)
Updated•16 years ago
|
Assignee: nobody → lusian
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment 4•16 years ago
|
||
No, those folders contain all of our xpcshell tests
Updated•16 years ago
|
Attachment #404795 -
Flags: review?(mak77) → review+
Updated•16 years ago
|
Attachment #404903 -
Flags: review?(mak77) → review-
Comment 5•16 years ago
|
||
Comment on attachment 404903 [details] [diff] [review]
Fix toolkit/components/places/tests that fail when werror is on
>diff --git a/toolkit/components/places/tests/queries/test_querySerialization.js b/toolkit/components/places/tests/queries/test_querySerialization.js
>--- a/toolkit/components/places/tests/queries/test_querySerialization.js
>+++ b/toolkit/components/places/tests/queries/test_querySerialization.js
>@@ -665,17 +665,18 @@ function choose(aSet, aHowMany, aCallbac
> * an nsINavHistoryQuery object
> * @param aQuery2
> * another nsINavHistoryQuery object
> * @return true if this switch is the same in both aQuery1 and aQuery2
> */
> function flagSwitchMatches(aQuery1, aQuery2)
> {
> if (aQuery1[this.flag] && aQuery2[this.flag]) {
>- for (let p in this.subswitches) {
>+ for (let i in this.subswitches) {
>+ let p = this.subswitches[i];
this change looks wrong, maybe you wanted this instead:
for each (let p in this.subswitches)
what kind of warning do you get from here? does using "for each" fix it?
>diff --git a/toolkit/components/places/tests/queries/test_sorting.js b/toolkit/components/places/tests/queries/test_sorting.js
>--- a/toolkit/components/places/tests/queries/test_sorting.js
>+++ b/toolkit/components/places/tests/queries/test_sorting.js
>@@ -317,16 +317,17 @@ tests.push({
> uri: "http://example.com/c",
> parentFolder: bmsvc.toolbarFolder,
> index: 3,
> title: "x",
> isInQuery: true },
>
> // if no URI (e.g., node is a folder), should fall back to title
> { isFolder: true,
>+ uri: "",
> parentFolder: bmsvc.toolbarFolder,
> index: 4,
> title: "a",
> isInQuery: true },
>
> // if URIs and dates are equal, should fall back to bookmark index
> { isBookmark: true,
> isDetails: true,
>@@ -334,16 +335,17 @@ tests.push({
> uri: "http://example.com/c",
> parentFolder: bmsvc.toolbarFolder,
> index: 5,
> title: "x",
> isInQuery: true },
>
> // if no URI and titles are equal, should fall back to bookmark index
> { isFolder: true,
>+ uri: "",
> parentFolder: bmsvc.toolbarFolder,
> index: 6,
> title: "a",
> isInQuery: true },
> ];
>
> this._sortedData = [
> this._unsortedData[4],
I'm not sure this change is correct, sounds like there is a bug in head_queries.js instead that tries to use uri property for a folder... that should be fixed rather than adding an empty uri properties to folders.
thank you very much for the cleanup, will be fine once you address my two concerns above.
Assignee | ||
Comment 6•16 years ago
|
||
Attachment #404903 -
Attachment is obsolete: true
Attachment #404949 -
Flags: review?(mak77)
Assignee | ||
Comment 7•16 years ago
|
||
Attachment #404949 -
Attachment is obsolete: true
Attachment #404955 -
Flags: review?(mak77)
Attachment #404949 -
Flags: review?(mak77)
Updated•16 years ago
|
Attachment #404955 -
Flags: review?(mak77) → review+
Comment 8•16 years ago
|
||
Comment on attachment 404955 [details] [diff] [review]
Address the comment 5
[Checkin: Comment 10+11]
Looks good, thanks!
Updated•16 years ago
|
Keywords: checkin-needed
Whiteboard: [2 patches to checkin]
Comment 9•16 years ago
|
||
the first patch had windows newlines, this fixes it.
Attachment #404795 -
Attachment is obsolete: true
Comment 10•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/3a8ee177f54c
http://hg.mozilla.org/mozilla-central/rev/aff8501ff328
thanks for looking into this.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [2 patches to checkin]
Target Milestone: --- → Firefox 3.7a1
Comment 11•16 years ago
|
||
i undid the for => for each change, seeing your changes i thought it was correct, but actually what we have there is correct, so i'm not sure which kind of warning are you getting from it.
for (let p in obj) is pretty valid and should not fail nor warn. please clarify the warning.
http://hg.mozilla.org/mozilla-central/rev/fcaa766c0991
Assignee | ||
Comment 12•16 years ago
|
||
(In reply to comment #11)
I paste the log here:
$ make SOLO_FILE="test_querySerialization.js" -C toolkit/components/places/test
s/ check-one
make: Entering directory `/c/mozilla-build/mozilla-central-devel/toolkit/compone
nts/places/tests'
c:/mozilla-build/python25/python2.5.exe -u /c/mozilla-build/mozilla-central-deve
l/config/pythonpath.py \
-I/c/mozilla-build/mozilla-central-devel/build \
/c/mozilla-build/mozilla-central-devel/testing/xpcshell/runxpcshelltes
ts.py \
--symbols-path=../../../../dist/crashreporter-symbols \
--test-path=test_querySerialization.js \
../../../../dist/bin/xpcshell \
../../../../_tests/xpcshell/test_places/autocomplete ../../../../_test
s/xpcshell/test_places/sync ../../../../_tests/xpcshell/test_places/bookmarks ..
/../../../_tests/xpcshell/test_places/queries ../../../../_tests/xpcshell/test_p
laces/unit
TEST-UNEXPECTED-FAIL | c:\mozilla-build\mozilla-central-devel\_tests\xpcshell\te
st_places\queries\test_querySerialization.js | test failed (with xpcshell return
code: 0), see following log:
>>>>>>>
TEST-INFO | (xpcshell/head.js) | test 1 pending
CHOOSING 1 SWITCHES
0
nsINavHistoryQuery.hasBeginTime
place:beginTime=1255090935104000
TEST-PASS | c:/mozilla-build/mozilla-central-devel/_tests/xpcshell/test_places/q
ueries/test_querySerialization.js | [serializeDeserialize : 805] 1 == 1
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | ReferenceError: reference to undefin
ed property aQuery1[p]
TEST-PASS | c:/mozilla-build/mozilla-central-devel/_tests/xpcshell/test_places/q
ueries/tail_queries.js | [null : 61] false == false
<<<<<<<
Comment 13•16 years ago
|
||
oh then i suppose the problem is that before using aQuery1[p] we should check if (p in aQuery1), can you try doing that (i think we need to do that also for aQuery 2)?
Comment 14•16 years ago
|
||
reopening because we miss one change, so we don't lose track of it
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 15•16 years ago
|
||
Passed the test! Please review the change.
Comment 16•16 years ago
|
||
Comment on attachment 405454 [details] [diff] [review]
test_querySerialization
[Checkin: Comment 17]
yeah, this is better and passes test, thanks
Attachment #405454 -
Flags: review+
Updated•16 years ago
|
Summary: [xpcshell-tests] some tests fail when 'werror' is on. → xpcshell-tests: some Places unit tests fail when 'werror' is on.
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Comment 17•16 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Assignee | ||
Comment 18•16 years ago
|
||
Attachment #409052 -
Flags: review?(mak77)
Assignee | ||
Updated•16 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•16 years ago
|
Attachment #409052 -
Flags: review?(mak77) → review+
Comment 19•16 years ago
|
||
Comment on attachment 409052 [details] [diff] [review]
test_database_sync_after_shutdown_with_removeAllPages.js
[Checkin: Comment 20]
oops, my fault :(
Comment 20•16 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 21•16 years ago
|
||
Attachment #413053 -
Flags: review?
Assignee | ||
Updated•16 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•16 years ago
|
Attachment #413053 -
Flags: review? → review+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Updated•16 years ago
|
Attachment #409052 -
Attachment description: test_database_sync_after_shutdown_with_removeAllPages.js → test_database_sync_after_shutdown_with_removeAllPages.js
[Checkin: Comment 20]
Updated•16 years ago
|
Attachment #405454 -
Attachment description: test_querySerialization → test_querySerialization
[Checkin: Comment 17]
Updated•16 years ago
|
Attachment #405437 -
Attachment description: first patch with correct newlines → first patch with correct newlines
[Checkin: Comment 10]
Updated•16 years ago
|
Attachment #404955 -
Attachment description: Address the comment 5 → Address the comment 5
[Checkin: Comment 10]
Updated•16 years ago
|
Attachment #404955 -
Attachment description: Address the comment 5
[Checkin: Comment 10] → Address the comment 5
[Checkin: Comment 10+11]
Updated•16 years ago
|
Flags: in-testsuite+
Version: unspecified → Trunk
Comment 22•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/5008731e77c0
i hope werror will be enabled soon so we can stop reopening bugs :)
thanks.
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Updated•16 years ago
|
Attachment #413053 -
Attachment description: test_419731.js: assignment to undeclared variable cc → test_419731.js: assignment to undeclared variable cc [Checkin: Comment 22]
Comment 23•16 years ago
|
||
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".
In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body contains places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.
Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.
Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Comment 24•16 years ago
|
||
Comment on attachment 404955 [details] [diff] [review]
Address the comment 5
[Checkin: Comment 10+11]
Approvals for 'utils.js' part:
No risk, just add 2 'let '.
Attachment #404955 -
Flags: approval1.9.2?
Attachment #404955 -
Flags: approval1.9.1.7?
Updated•16 years ago
|
Keywords: checkin-needed
Whiteboard: [c-n: m-1.9.2, m-1.9.1]
Comment 25•16 years ago
|
||
Comment on attachment 404955 [details] [diff] [review]
Address the comment 5
[Checkin: Comment 10+11]
Clearing 1.9.1 approval flag, it's not clear this fits the stability branch criteria nor has it landed on newer branches
Attachment #404955 -
Flags: approval1.9.1.8?
Comment 26•15 years ago
|
||
I wasn't sure what needed checkin here, and to where, so I left it...
Updated•15 years ago
|
Attachment #404955 -
Flags: approval1.9.2? → approval1.9.2.2?
Updated•15 years ago
|
Whiteboard: [c-n: m-1.9.2, m-1.9.1] → [c-n: m-1.9.2, m-1.9.1][still needs approvals]
Comment 27•15 years ago
|
||
please add back checkin-needed once the patches are approved for specific branches.
Keywords: checkin-needed
Updated•15 years ago
|
Attachment #404955 -
Flags: approval1.9.2.2? → approval1.9.2.3?
Comment 28•15 years ago
|
||
Comment on attachment 404955 [details] [diff] [review]
Address the comment 5
[Checkin: Comment 10+11]
Not going to make 1.9.2.2
Comment 29•15 years ago
|
||
Comment on attachment 404955 [details] [diff] [review]
Address the comment 5
[Checkin: Comment 10+11]
Clearing old approval requests now that 1.9.2.4 has shipped. If you believe this patch is still necessary on the 1.9.2 branch please re-request approval along with a risk/benefit analysis explaining why we need it.
Attachment #404955 -
Flags: approval1.9.2.4?
Updated•13 years ago
|
Whiteboard: [c-n: m-1.9.2, m-1.9.1][still needs approvals]
You need to log in
before you can comment on or make changes to this bug.
Description
•