Closed Bug 520743 Opened 10 years ago Closed 10 years ago

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

Categories

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

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
Firefox 3.7a1

People

(Reporter: lusian, Assigned: lusian)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 3 obsolete files)

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
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.
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?
Attachment #404795 - Flags: review?(mak77)
Attachment #404903 - Flags: review?(mak77)
Assignee: nobody → lusian
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
No, those folders contain all of our xpcshell tests
Attachment #404795 - Flags: review?(mak77) → review+
Attachment #404903 - Flags: review?(mak77) → review-
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.
Attachment #404903 - Attachment is obsolete: true
Attachment #404949 - Flags: review?(mak77)
Attachment #404949 - Attachment is obsolete: true
Attachment #404955 - Flags: review?(mak77)
Attachment #404949 - Flags: review?(mak77)
Attachment #404955 - Flags: review?(mak77) → review+
Comment on attachment 404955 [details] [diff] [review]
Address the comment 5
[Checkin: Comment 10+11]

Looks good, thanks!
Keywords: checkin-needed
Whiteboard: [2 patches to checkin]
the first patch had windows newlines, this fixes it.
Attachment #404795 - Attachment is obsolete: true
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: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [2 patches to checkin]
Target Milestone: --- → Firefox 3.7a1
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
(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

  <<<<<<<
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)?
reopening because we miss one change, so we don't lose track of it
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Passed the test!  Please review the change.
Comment on attachment 405454 [details] [diff] [review]
test_querySerialization
[Checkin: Comment 17]

yeah, this is better and passes test, thanks
Attachment #405454 - Flags: review+
Summary: [xpcshell-tests] some tests fail when 'werror' is on. → xpcshell-tests: some Places unit tests fail when 'werror' is on.
Blocks: 477583
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/d6f8bac7328d
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Blocks: 524781
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #409052 - Flags: review?(mak77) → review+
Comment on attachment 409052 [details] [diff] [review]
test_database_sync_after_shutdown_with_removeAllPages.js
[Checkin: Comment 20]

oops, my fault :(
http://hg.mozilla.org/mozilla-central/rev/d7e464f89e7c

thank you.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #413053 - Flags: review? → review+
Keywords: checkin-needed
Attachment #409052 - Attachment description: test_database_sync_after_shutdown_with_removeAllPages.js → test_database_sync_after_shutdown_with_removeAllPages.js [Checkin: Comment 20]
Attachment #405454 - Attachment description: test_querySerialization → test_querySerialization [Checkin: Comment 17]
Attachment #405437 - Attachment description: first patch with correct newlines → first patch with correct newlines [Checkin: Comment 10]
Attachment #404955 - Attachment description: Address the comment 5 → Address the comment 5 [Checkin: Comment 10]
Attachment #404955 - Attachment description: Address the comment 5 [Checkin: Comment 10] → Address the comment 5 [Checkin: Comment 10+11]
Flags: in-testsuite+
Version: unspecified → Trunk
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: 10 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #413053 - Attachment description: test_419731.js: assignment to undeclared variable cc → test_419731.js: assignment to undeclared variable cc [Checkin: Comment 22]
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 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?
Keywords: checkin-needed
Whiteboard: [c-n: m-1.9.2, m-1.9.1]
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?
I wasn't sure what needed checkin here, and to where, so I left it...
Attachment #404955 - Flags: approval1.9.2? → approval1.9.2.2?
Whiteboard: [c-n: m-1.9.2, m-1.9.1] → [c-n: m-1.9.2, m-1.9.1][still needs approvals]
please add back checkin-needed once the patches are approved for specific branches.
Keywords: checkin-needed
Attachment #404955 - Flags: approval1.9.2.2? → approval1.9.2.3?
Comment on attachment 404955 [details] [diff] [review]
Address the comment 5
[Checkin: Comment 10+11]

Not going to make 1.9.2.2
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?
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.