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)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
Firefox 3.7a1

People

(Reporter: lusian, Assigned: lusian)

References

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
Status: ASSIGNED → RESOLVED
Closed: 16 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
Status: REOPENED → RESOLVED
Closed: 16 years ago16 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 :(
Status: REOPENED → RESOLVED
Closed: 16 years ago16 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: 16 years ago16 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.

Attachment

General

Created:
Updated:
Size: