Closed Bug 468341 Opened 16 years ago Closed 16 years ago

Some toolkit places tests fail on SeaMonkey unit test boxes

Categories

(Toolkit :: Places, defect)

Product:
Toolkit
Component:
Places
Version:
1.9.1 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.1b3

People

(Reporter: kairo, Assigned: kairo)

References

(Blocks 1 open bug)

Details

(Keywords: fixed1.9.1)

Attachments

(2 files, 2 obsolete files)

address comments [Checkin: Comment 21]
7.45 KB, patch
sdwilsh
: review+
Details | Diff | Splinter Review
really fix test_423060.xul, v2
2.36 KB, patch
sdwilsh
: review+
Details | Diff | Splinter Review
We landed places history for SeaMonkey today, which enables MOZ_PLACES in our builds. Apparently some places tests fail for us, possibly in all cases where they're not executed on Firefox. We should either fix the tests to be able to run generically or ifdef them in a way that only Firefox executes them. The failures are: ***** Linux comm-central dep unit test **** ----- TUnit ----- TEST-UNEXPECTED-FAIL | ../../../../_tests/xpcshell-simple/test_places/autocomplete/test_special_search.js | test failed, see log ../../../../_tests/xpcshell-simple/test_places/autocomplete/test_special_search.js.log: >>>>>>> Adding page/book/tag: 0, 0, , , , uri=http://url/, title=title Adding page/book/tag: 1, 1, , , , uri=http://url/2, title=foo.bar Adding page/book/tag: 2, 0, , , , uri=http://foo.bar/, title=title Adding page/book/tag: 3, 1, , , , uri=http://foo.bar/2, title=foo.bar Adding page/book/tag: 4, 0, 0, , , uri=http://url/star, title=title, book=title Adding page/book/tag: 5, 1, 1, , , uri=http://url/star/2, title=foo.bar, book=foo.bar Adding page/book/tag: 6, 0, 0, , , uri=http://foo.bar/star, title=title, book=title Adding page/book/tag: 7, 1, 1, , , uri=http://foo.bar/star/2, title=foo.bar, book=foo.bar Adding page/book/tag: 8, 0, 0, 1, , uri=http://url/tag, title=title, book=title, tags=foo.bar Adding page/book/tag: 9, 1, 1, 1, , uri=http://url/tag/2, title=foo.bar, book=foo.bar, tags=foo.bar Adding page/book/tag: 10, 0, 0, 1, , uri=http://foo.bar/tag, title=title, book=title, tags=foo.bar Adding page/book/tag: 11, 1, 1, 1, , uri=http://foo.bar/tag/2, title=foo.bar, book=foo.bar, tags=foo.bar *** test pending *** test pending 0: History restrict Searching for.. ^ *** test finished *** running event loop Looking for http://foo.bar/tag, title foo.bar in expected results... Got it at index 5!! Looking for http://url/star/2, foo.bar in expected results... Got it at index 4!! Looking for http://foo.bar/2, foo.bar in expected results... Got it at index 3!! Looking for http://foo.bar/, title in expected results... Got it at index 2!! Looking for http://url/2, foo.bar in expected results... Got it at index 1!! Looking for http://url/, title in expected results... Got it at index 0!! *** test pending 1: Star restrict Searching for.. * *** test finished Looking for http://foo.bar/tag, title foo.bar in expected results... Got it at index 6!! Looking for http://url/star/2, foo.bar in expected results... Got it at index 1!! *** exiting *** TEST-UNEXPECTED-FAIL | ../../../../_tests/xpcshell-simple/test_places/autocomplete/head_autocomplete.js | 2 == 8 JS frame :: /builds/slave/comm-central-linux/build/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_throw :: line 101 JS frame :: /builds/slave/comm-central-linux/build/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_check_eq :: line 120 JS frame :: ../../../../_tests/xpcshell-simple/test_places/autocomplete/head_autocomplete.js :: anonymous :: line 127 *** FAIL *** ----- mochitest ----- *** 68184 INFO Running /tests/toolkit/components/places/tests/test_bug_405924.html... *** 68185 INFO TEST-PASS | /tests/toolkit/components/places/tests/test_bug_405924.html | Access Ci *** 68186 INFO TEST-PASS | /tests/toolkit/components/places/tests/test_bug_405924.html | Access Cc *** 68187 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/places/tests/test_bug_405924.html | Check for the correct message *** 68188 INFO TEST-KNOWN-FAIL | /tests/toolkit/components/places/tests/test_bug_405924.html | This should throw: bug 428585 *** 68189 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/places/tests/test_bug_405924.html | Check for the correct message ----- mochichrome ----- *** 5178 INFO Running chrome://mochikit/content/chrome/toolkit/components/places/tests/chrome/test_423060.xul... *** 5179 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/toolkit/components/places/tests/chrome/test_423060.xul | Error thrown during test: Cc[wccrID] is undefined - got 0, expected 1 ***** MacOSX 10.4 comm-central dep unit test **** ----- TUnit ----- <same as Linux> ----- mochitest ----- <same as Linux, additionally:> *** 68505 INFO Running /tests/toolkit/components/places/tests/test_bug_411966.html... *** 68506 INFO TEST-PASS | /tests/toolkit/components/places/tests/test_bug_411966.html | Access Ci *** 68507 INFO TEST-PASS | /tests/toolkit/components/places/tests/test_bug_411966.html | Access Cc *** 68508 INFO TEST-PASS | /tests/toolkit/components/places/tests/test_bug_411966.html | Could not get History Service *** 68509 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/places/tests/test_bug_411966.html | Error thrown during test: uncaught exception: [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]" nsresult: "0x80004002 (NS_NOINTERFACE)" location: "JS frame :: http://localhost:8888/tests/toolkit/components/places/tests/bug_411966/redirect.js :: <TOP_LEVEL> :: line 51" data: no] - got 0, expected 1 *** 68510 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/places/tests/test_bug_411966.html | Error thrown during test: ghist is undefined - got 0, expected 1 ----- mochichrome ----- <same as Linux> ***** Win2k3 comm-central dep unit test **** ----- TUnit ----- <same as Linux> ----- mochitest ----- <same as Linux> ----- mochichrome ----- <doesn't run as mochitest doesn't end correctly>
I manually ended mochitest on the Windows machine from RDP, so that mochichrome would run and can confirm that the failure happening on Linux and Mac also happens there. To make sure those failures aren't caused by files sticking around from before the checkin, I now killed dist/ and _tests/ on all three machines and they're respinning with that now.
The autocomplete tests assume you are using the autocomplete provider in toolkit/components/places, which you aren't, so I would expect them to fail. Not sure how you plan on making them not run since unit tests don't have any means of ifdefing
The test failures persist after that cleanup, so they are real. Might some of those tests rely e.g. on MOZ_PLACES_BOOKMARKS which is not explicitely turned on in SeaMonkey? Or on some FF-specific code? I'm not cdompletely sure what you mean with the autocomplete provider - we have switched to having history autocomplete be driven by the toolkit places code now. CCing Neil who knows more than anybody else about what we are using wrt autocomplete.
(In reply to comment #2) > The autocomplete tests assume you are using the autocomplete provider in > toolkit/components/places, which you aren't, Yes we are, we're using nsNavHistoryAutoComplete.cpp now. I suspect "book" and "tags" refer to Places bookmarks which we don't have.
If it's test code in toolkit's places stuff, it should only test code that is available with it. It might be that those tests really test against stuff that isn't compiled without MOZ_PLACES_BOOKMARKS though, in which case those tests should not run without that flag.
i could be blind but looking at mxr i can't find any code using MOZ_PLACES_BOOKMARKS, so i suspect the failure has another origin.
OK, I just tried locally and I'm seeing those test failures here as well, at least the unit test thing. It seems to expect 2 results and gets 8 somehow. When I disable that test, I see two more failures in unit/: TEST-UNEXPECTED-FAIL | ../../../../_tests/xpcshell-simple/test_places/unit/test_000_frecency.js | test failed, see log ../../../../_tests/xpcshell-simple/test_places/unit/test_000_frecency.js.log: >>>>>>> *** test pending *** test pending *** test finished *** running event loop *** exiting *** TEST-UNEXPECTED-FAIL | ../../../../_tests/xpcshell-simple/test_places/unit/test_000_frecency.js | 20 == 21 JS frame :: /mnt/mozilla/comm-central/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_throw :: line 101 JS frame :: /mnt/mozilla/comm-central/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_check_eq :: line 120 JS frame :: ../../../../_tests/xpcshell-simple/test_places/unit/test_000_frecency.js :: anonymous :: line 263 *** FAIL *** <<<<<<< TEST-UNEXPECTED-FAIL | ../../../../_tests/xpcshell-simple/test_places/unit/test_408221.js | test failed, see log ../../../../_tests/xpcshell-simple/test_places/unit/test_408221.js.log: >>>>>>> *** test pending *** test pending *** test finished *** running event loop *** exiting *** TEST-UNEXPECTED-FAIL | ../../../../_tests/xpcshell-simple/test_places/unit/test_408221.js | 3 == 4 JS frame :: /mnt/mozilla/comm-central/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_throw :: line 101 JS frame :: /mnt/mozilla/comm-central/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_check_eq :: line 120 JS frame :: ../../../../_tests/xpcshell-simple/test_places/unit/test_408221.js :: anonymous :: line 126 *** FAIL *** <<<<<<< TEST-UNEXPECTED-FAIL | ../../../../_tests/xpcshell-simple/test_places/unit/test_history_autocomplete_tags.js | test failed, see log ../../../../_tests/xpcshell-simple/test_places/unit/test_history_autocomplete_tags.js.log: >>>>>>> *** test pending Searching for 'foo' *** test pending *** test finished *** running event loop *** exiting *** TEST-UNEXPECTED-FAIL | ../../../../_tests/xpcshell-simple/test_places/unit/test_history_autocomplete_tags.js | 3 == 4 JS frame :: /mnt/mozilla/comm-central/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_throw :: line 101 JS frame :: /mnt/mozilla/comm-central/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_check_eq :: line 120 JS frame :: ../../../../_tests/xpcshell-simple/test_places/unit/test_history_autocomplete_tags.js :: anonymous :: line 129 *** FAIL *** <<<<<<< TEST-UNEXPECTED-FAIL | ../../../../_tests/xpcshell-simple/test_places/unit/test_isvisited.js | test failed, see log ../../../../_tests/xpcshell-simple/test_places/unit/test_isvisited.js.log: >>>>>>> *** test pending [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIIOService.newURI]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: ../../../../_tests/xpcshell-simple/test_places/unit/head_bookmarks.js :: uri :: line 90" data: no] *** FAIL *** <<<<<<< All other unit tests pass, including the whole queries/, bookmarks/, and sync/ hierarchies. Perhaps we can find something common to all those failures.
Hah, I got it! At least I can fix everything in TUnit but the test_isvisited.js failure by setting |pref("browser.urlbar.search.sources", 3);| (search history+bookmarks) instead of the SeaMonkey default of 1, which is history only.
Ugh. tests/chrome/test_423060.xul relies on a dialog wording from browser/locales/en-US/chrome/overrides/appstrings.properties while any other app than Firefox (including SeaMonkey) uses the standard wording from dom/locales/en-US/chrome/appstrings.properties ("%S is not a registered protocol.") I have a fixed version that looks for both variants locally.
Ah, and tests/chrome/test_423060.xul tests a service defined in browser/ (web feed handler), we probably should just abort the test if the contract ID for it is unknown.
The only thing that might be a SeaMonkey problem is the test_isvisited.js problem, it throws on the imap:// URL - as mailnews defines this protocol, we either have an error there or the URL may be invalid or so, not sure what makes nsIIOService.newURI throw a NS_ERROR_FAILURE. As we don't care too much about that in the test though, we should be safe to catch that exception and just log it, I think.
Attached patch make tests pass on SeaMonkey (obsolete) — — Splinter Review
This patch makes all places tests pass (or not fail) for SeaMonkey, doing the following: 1) make sure browser.urlbar.search.sources is set to 3 for all autocomplete tests that need to search for more than just history items 2) abort test involving web feed handling when web feed handler is not present 3) make the check for the "protocol not defined" message work with the standard DOM message as well as the Firefox override message 4) ignore and log throwing of nsIIOService.newURI() - I'm told our behavior is probably correct for a client as us that knows about the protocol but doesn't have that IMAP account defined.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #351946 - Flags: review?(mak77)
Attachment #351946 - Flags: review?(mak77) → review-
Comment on attachment 351946 [details] [diff] [review] make tests pass on SeaMonkey >+ // always search in history + bookmarks, no matter what the default is >+ Cc["@mozilla.org/preferences-service;1"] >+ .getService(Ci.nsIPrefBranch) >+ .setIntPref("browser.urlbar.search.sources", 3); This doesn't follow the style in the file >+ // always search in history + bookmarks, no matter what the default is >+ Cc["@mozilla.org/preferences-service;1"] >+ .getService(Ci.nsIPrefBranch) >+ .setIntPref("browser.urlbar.search.sources", 3); ditto >- var cantAddUri = uri(currentURL); >- add_uri_to_history(cantAddUri); >- do_check_false(gh.isVisited(cantAddUri)); >+ try { >+ var cantAddUri = uri(currentURL); >+ add_uri_to_history(cantAddUri); >+ do_check_false(gh.isVisited(cantAddUri)); >+ } >+ catch(e) { >+ print("Exception thrown for '" + currentURL + "', ignored."); >+ } This also has the effect of swallowing any real error, which defeats the test by and large
(In reply to comment #13) > (From update of attachment 351946 [details] [diff] [review]) > >+ // always search in history + bookmarks, no matter what the default is > >+ Cc["@mozilla.org/preferences-service;1"] > >+ .getService(Ci.nsIPrefBranch) > >+ .setIntPref("browser.urlbar.search.sources", 3); > This doesn't follow the style in the file I feared that but I know the toolkit style too little, what is really correct? Cc["@mozilla.org/preferences-service;1"]. getService(Ci.nsIPrefBranch). setIntPref("browser.urlbar.search.sources", 3); Is it that? > >- var cantAddUri = uri(currentURL); > >- add_uri_to_history(cantAddUri); > >- do_check_false(gh.isVisited(cantAddUri)); > >+ try { > >+ var cantAddUri = uri(currentURL); > >+ add_uri_to_history(cantAddUri); > >+ do_check_false(gh.isVisited(cantAddUri)); > >+ } > >+ catch(e) { > >+ print("Exception thrown for '" + currentURL + "', ignored."); > >+ } > This also has the effect of swallowing any real error, which defeats the test > by and large By what do you judge that? I feared it might, but then what we are caring for in the test is that we don't record those as visited, and when I added a normal http:// URL to the set we're testing here, the test still failed as expected. Only after that, I considered this fix as safe.
(In reply to comment #14) > Cc["@mozilla.org/preferences-service;1"]. > getService(Ci.nsIPrefBranch). > setIntPref("browser.urlbar.search.sources", 3); Except that looks like three lines of code. Store the pref branch in a variable, then do the setIntPref. > By what do you judge that? I feared it might, but then what we are caring for > in the test is that we don't record those as visited, and when I added a normal > http:// URL to the set we're testing here, the test still failed as expected. > Only after that, I considered this fix as safe. add_uri_to_history can throw, and we'd want that to be a failure.
This version of the patch should address all of sdwilsh's comments on the previous one, thanks for looking into it! :)
Attachment #351946 - Attachment is obsolete: true
Attachment #351965 - Flags: review?(sdwilsh)
Comment on attachment 351965 [details] [diff] [review] address comments [Checkin: Comment 21] >+ // always search in history + bookmarks, no matter what the default is >+ prefs.setIntPref("browser.urlbar.search.sources", 3); >+ (in all tests) Don't we want to save and restore the previous value ? Though I assume depending tests could be responsible for setting their own needed value... >+ var wccrID = "@mozilla.org/embeddor.implemented/web-content-handler-registrar;1"; >+ /* abort the test if web feed handlers are not available */ >+ if (!Cc[wccrID]) { >+ SimpleTest.finish() >+ Shouldn't we print some kind of info/todo, as a reminder ? >- var cantAddUri = uri(currentURL); >- add_uri_to_history(cantAddUri); >- do_check_false(gh.isVisited(cantAddUri)); >+ try { >+ var cantAddUri = uri(currentURL); Clearer with |var cantAddUri = null;| before the |try|. >+ } >+ catch(e) { >+ print("Exception thrown for '" + currentURL + "', ignored."); >+ } >+ if (cantAddUri) {
(In reply to comment #17) > (From update of attachment 351965 [details] [diff] [review]) > >+ // always search in history + bookmarks, no matter what the default is > >+ prefs.setIntPref("browser.urlbar.search.sources", 3); > > (in all tests) > Don't we want to save and restore the previous value ? No, each test run anew and doesn't save any prefs it sets anywhere, so each test starts with the defaults. > >+ var wccrID = "@mozilla.org/embeddor.implemented/web-content-handler-registrar;1"; > >+ /* abort the test if web feed handlers are not available */ > >+ if (!Cc[wccrID]) { > >+ SimpleTest.finish() > >+ > > Shouldn't we print some kind of info/todo, as a reminder ? No, there's nothing to remind us of, it's no bug/problem if someone doesn't support web feed handlers, the test just doesn't apply and therefore aborts early, without failing/succeeding/todo or whatever. > >- var cantAddUri = uri(currentURL); > >- add_uri_to_history(cantAddUri); > >- do_check_false(gh.isVisited(cantAddUri)); > >+ try { > >+ var cantAddUri = uri(currentURL); > > Clearer with |var cantAddUri = null;| before the |try|. I don't think it really makes a difference, but I'll let the reviewer decide that :)
(In reply to comment #18) > No, each test run anew and doesn't save any prefs it sets anywhere, so each > test starts with the defaults. Right ! I was confusing with mochitests :-< > No, there's nothing to remind us of, it's no bug/problem if someone doesn't > support web feed handlers, the test just doesn't apply and therefore aborts > early, without failing/succeeding/todo or whatever. Since the test is started, I would expect some kind of (info) feedback, but I won't argue more over it. > I don't think it really makes a difference, but I'll let the reviewer decide > that :) k; I just ate to look for |var| coming out of (try) block, and in this case I wondered if the catch+if case was safe (in the loop).
Comment on attachment 351965 [details] [diff] [review] address comments [Checkin: Comment 21] r=sdwilsh, with a comment on why we need to do the try-catch
Attachment #351965 - Flags: review?(sdwilsh) → review+
Ugh, I had a spurious { in the test_423060.xul part of the patch and we obviously all missed it, so it caused some test bustage on Firefox, sorry for that :( Pushed as http://hg.mozilla.org/mozilla-central/rev/ee8547127768 fixed up by http://hg.mozilla.org/mozilla-central/rev/4897aa66c45c on trunk and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/585ae80f5771 fixed up by http://hg.mozilla.org/releases/mozilla-1.9.1/rev/583ed3ab7afd on branch.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Keywords: fixed1.9.1
Target Milestone: --- → mozilla1.9.1b3
It looks like another test needs the same update: { http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1228916730.1228921173.32271.gz Linux comm-central dep unit test on 2008/12/10 05:45:30 *** 5181 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/toolkit/components/viewsource/test/test_428653.xul | Error thrown during test: Cc[wccrID] is undefined - got 0, expected 1 }
(In reply to comment #22) http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1228917806.1228922357.2793.gz Win2k3 comm-central dep unit test on 2008/12/10 06:03:26 has that (new) error too... (and only that one :-))
(In reply to comment #22) Without surprise: http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1228919033.1228924996.9488.gz MacOSX 10.4 comm-central dep unit test on 2008/12/10 06:23:53 *** Said code is only in test_423060.xul ... which confirms the strangeness KaiRo noted iirc. But a hint could be that test_423060.xul runs immediately before test_428653.xul (and (Linux, one-time only) failing test_autocomplete2.xul) It would look like the error is leaking from one test to the next, somehow...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch really fix test_423060.xul (obsolete) — — Splinter Review
Actually, the bug I had in my fixup for test_423060.xul was what made it pass on SeaMonkey, if I get that right. :-/ In any case, calling SimpleTest.finish() alone doesn't stop us from executing the rest of the test, and as the exception is then thrown after the .finish(), it gets reported for the next executed test. To fix this, we need to return early from the script execution. This still makes us call the unload function, so we also need to return early from that one.
Attachment #352651 - Flags: review?(sdwilsh)
Comment on attachment 352651 [details] [diff] [review] really fix test_423060.xul > SimpleTest.waitForExplicitFinish(); > > const Cc = Components.classes; > const Ci = Components.interfaces; > > var wccrID = "@mozilla.org/embeddor.implemented/web-content-handler-registrar;1"; > /* abort the test if web feed handlers are not available */ >- if (!Cc[wccrID]) >- SimpleTest.finish() >+ if (!Cc[wccrID]) { >+ SimpleTest.finish(); >+ return; >+ } I think |SimpleTest.waitForExplicitFinish();| could be moved to after the |if|, so |SimpleTest.finish();| wouldn't be needed. > function finishUp() { >+ /* onload still arrives here, even when we abort the test */ >+ if (!Cc[wccrID]) >+ return I think |onload="finishUp();"| could be added+removed dynamically instead, so doubling the |if+return| wouldn't be needed.
Attachment #351965 - Attachment description: address comments → address comments [Checkin: Comment 21]
(In reply to comment #25) > Actually, the bug I had in my fixup for test_423060.xul was what made it pass > on SeaMonkey, if I get that right. :-/ Yeah, the JS syntax error must have been actually aborting the script ;-> > In any case, calling SimpleTest.finish() alone doesn't stop us from executing > the rest of the test, and as the exception is then thrown after the .finish(), Yep, finish() means "the test/part we care about is done now", but the script can nonetheless continue (to do some cleanup/...) :-| Actually, it really means "framework don't have to (explicitly) wait any longer before starting next test".
Serge, somehow you keep pointing out a big part of the thoughts my mind went through when coming up with the patches, and esp. those parts that is discarded without explicitely listing all small details here about why I did that, because I relied on those being implicitly clear to those involved in reviewing and dealing with those tests. (And isn't comment #27 just the same as what I pointed out in comment #25, just with other words? :p ) (In reply to comment #26) > I think |SimpleTest.waitForExplicitFinish();| could be moved to after the |if|, > so |SimpleTest.finish();| wouldn't be needed. Yes, it could. I decided not to do that to keep the changes smaller, but I'll defer to it if sdwilsh wants. > I think |onload="finishUp();"| could be added+removed dynamically instead, > so doubling the |if+return| wouldn't be needed. Yes, but I figured that would look messier and harder to understand for someone else reading the test than just exiting the function prematurely.
(In reply to comment #28) > I relied on those being implicitly clear to those involved in reviewing > and dealing with those tests. I can't read your mind yet :-> > (And isn't comment #27 just the same as what I pointed out in comment #25, just > with other words? :p ) Yes, simple confirmation.
Blocks: SmTestFail
(In reply to comment #28) > > I think |SimpleTest.waitForExplicitFinish();| could be moved to after the |if|, > > so |SimpleTest.finish();| wouldn't be needed. > > Yes, it could. I decided not to do that to keep the changes smaller, but I'll > defer to it if sdwilsh wants. Yes please
This version of the fixup to test_423060.xul moves the SimpleTest.waitForExplicitFinish(); after the if that aborts setting up the test. We still need to break the onload function early as well, though.
Attachment #352651 - Attachment is obsolete: true
Attachment #352808 - Flags: review?(sdwilsh)
Attachment #352651 - Flags: review?(sdwilsh)
Comment on attachment 352808 [details] [diff] [review] really fix test_423060.xul, v2 r=sdwilsh
Attachment #352808 - Flags: review?(sdwilsh) → review+
Shawn, have you actually tested this in Firefox? If yes and it worked, then I'm convinced and I'll try checking it in. My Minefield crashes on startup so I can't test, but the Shiretoko on the other computer choked on the non-function return :( If that's really the case, then I give up, revert this test to what it was before and go with a Makefile hack excluding it from just SeaMonkey.
Actually, looking at the bug in question, this shouldn't even be a places test - it's fully testing feed handling. This should be moved into toolkit/components/feeds, and that also means I'm not a valid peer. I don't even know how that test got into places in the first place...
Blocks: 469593
sayrer probably dropped it there to be able to just reuse the demohandler and sample feed from there, but putting a toolkit feeds test that heavily uses browser/ feed features in places must have called for problems from the beginning. This look quite dirty to me. I filed bug 469593 on that test, let's resolved the bug here, as the real places tests are indeed fixed.
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Blocks: 471886
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: