520743 - xpcshell-tests: some Places unit tests fail when 'werror' is on.

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, trivial)

Description

[Jae-Seong Lee-Russo]

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

Comment 1

[Jae-Seong Lee-Russo]

Attachment: Fix tests that fail when werror is on

Comment 2

[Marco Bonardo [::mak] (Away 7-25 Aug)]

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.

Comment 3

[Jae-Seong Lee-Russo]

Attachment: Fix toolkit/components/places/tests that fail when werror is on

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?

Comment 4

[Marco Bonardo [::mak] (Away 7-25 Aug)]

No, those folders contain all of our xpcshell tests

Comment 5

[Marco Bonardo [::mak] (Away 7-25 Aug)]

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.

Comment 6

[Jae-Seong Lee-Russo]

Attachment: Fix toolkit/components/places/tests that fail when werror is on

Comment 7

[Jae-Seong Lee-Russo]

Attachment: Address the comment 5 [Checkin: Comment 10+11]

Comment 8

[Marco Bonardo [::mak] (Away 7-25 Aug)]

Comment on attachment 404955 [details] [diff] [review]
Address the comment 5
[Checkin: Comment 10+11]

Looks good, thanks!

Comment 9

[Marco Bonardo [::mak] (Away 7-25 Aug)]

Attachment: first patch with correct newlines [Checkin: Comment 10]

the first patch had windows newlines, this fixes it.

Comment 10

[Marco Bonardo [::mak] (Away 7-25 Aug)]

http://hg.mozilla.org/mozilla-central/rev/3a8ee177f54c
http://hg.mozilla.org/mozilla-central/rev/aff8501ff328

thanks for looking into this.

Comment 11

[Marco Bonardo [::mak] (Away 7-25 Aug)]

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

Comment 12

[Jae-Seong Lee-Russo]

(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

[Marco Bonardo [::mak] (Away 7-25 Aug)]

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

[Marco Bonardo [::mak] (Away 7-25 Aug)]

reopening because we miss one change, so we don't lose track of it

Comment 15

[Jae-Seong Lee-Russo]

Attachment: test_querySerialization [Checkin: Comment 17]

Passed the test!  Please review the change.

Comment 16

[Marco Bonardo [::mak] (Away 7-25 Aug)]

Comment on attachment 405454 [details] [diff] [review]
test_querySerialization
[Checkin: Comment 17]

yeah, this is better and passes test, thanks

Comment 17

[Marco Bonardo [::mak] (Away 7-25 Aug)]

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

Comment 18

[Jae-Seong Lee-Russo]

Attachment: test_database_sync_after_shutdown_with_removeAllPages.js [Checkin: Comment 20]

Comment 19

[Marco Bonardo [::mak] (Away 7-25 Aug)]

Comment on attachment 409052 [details] [diff] [review]
test_database_sync_after_shutdown_with_removeAllPages.js
[Checkin: Comment 20]

oops, my fault :(

Comment 20

[Marco Bonardo [::mak] (Away 7-25 Aug)]

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

thank you.

Comment 21

[Jae-Seong Lee-Russo]

Attachment: test_419731.js: assignment to undeclared variable cc [Checkin: Comment 22]

Comment 22

[Marco Bonardo [::mak] (Away 7-25 Aug)]

http://hg.mozilla.org/mozilla-central/rev/5008731e77c0

i hope werror will be enabled soon so we can stop reopening bugs :)
thanks.

Comment 23

[Gervase Markham [:gerv] (not reading bugmail)]

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

Comment 24

[Serge Gautherie (:sgautherie)]

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 '.

Comment 25

[Daniel Veditz [:dveditz]]

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

Comment 26

[Nochum Sossonko [:Natch]]

I wasn't sure what needed checkin here, and to where, so I left it...

Comment 27

[Marco Bonardo [::mak] (Away 7-25 Aug)]

please add back checkin-needed once the patches are approved for specific branches.

Comment 28

[Johnathan Nightingale [:johnath]]

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

[Daniel Veditz [:dveditz]]

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.