Closed Bug 470707 Opened 12 years ago Closed 12 years ago

[SeaMonkey] test_download_history.js fails now

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: sgautherie, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Keywords: regression, verified1.9.1)

Attachments

(1 file, 2 obsolete files)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20081217 SeaMonkey/2.0a3pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/4a4a42520901
 +http://hg.mozilla.org/comm-central/rev/877154dd96a2 + bug 469606 patch)

{
*** test pending
*** test finished
*** exiting
*** PASS ***
}

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20081221 SeaMonkey/2.0a3pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/b839ff0630c6
 +http://hg.mozilla.org/comm-central/rev/2a4c4c1f0feb + bug 469606 patch)

{
*** test pending
TypeError: tests is undefined
*** FAIL ***
[...]
../../../../_tests/xpcshell-simple/test_places/unit/test_download_history.js:53: TypeError: Cc['@mozilla.org/privatebrowsing;1'] is undefined
}

This is blamed to bug 458849.
i suppose that's for the lack of provateBrowsing mode, probably for now the best fix would be to split the test and put the pb part into private browsing unit tests, but Places should probably provide an in_memory_database mode and pb should use it, because actually we have code in toolkit dependant on pb that is a browser component.
Yeah, this bug case is much like bug 469593's :->
Flags: wanted1.9.2?
Attached patch patch v1 (obsolete) — Splinter Review
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attachment #354152 - Flags: review?(sdwilsh)
Attachment #354152 - Flags: review?(ehsan.akhgari)
Attachment #354152 - Flags: review?(ehsan.akhgari) → review-
Comment on attachment 354152 [details] [diff] [review]
patch v1

I'm not a peer, but I really don't like these tests to move into browser/, when they're actually testing the download manager code functionality.

The correct approach here would be to wrap the code to get the PB service in a try/catch block, and only test the private browsing part if the private browsing service actually exists.

For an example of this, check out <http://hg.mozilla.org/mozilla-central/file/tip/toolkit/components/places/tests/unit/test_248970.js>.
s/download manager/places/ of course.  :-)
(In reply to comment #4)
> (From update of attachment 354152 [details] [diff] [review])
> I'm not a peer, but I really don't like these tests to move into browser/, when
> they're actually testing the download manager code functionality.

that code path is only pertinent to private browsing, and since it is not a toolkit component the only available place to test that is with pb tests.

> 
> The correct approach here would be to wrap the code to get the PB service in a
> try/catch block, and only test the private browsing part if the private
> browsing service actually exists.

that would work but since private browsing is not part of toolkit would be wrong imho.
or maybe we could move it to browser/places tests since has a dependancy on a browser component...
(In reply to comment #6)
> that code path is only pertinent to private browsing, and since it is not a
> toolkit component the only available place to test that is with pb tests.

That part of the private browsing functionality is implemented in toolkit, not in browser/components/privatebrowsing.

> > The correct approach here would be to wrap the code to get the PB service in a
> > try/catch block, and only test the private browsing part if the private
> > browsing service actually exists.
> 
> that would work but since private browsing is not part of toolkit would be
> wrong imho.

Since we don't have a central place for the private browsing code to live, we left the tests for each component's functionality along with the component itself.

(In reply to comment #7)
> or maybe we could move it to browser/places tests since has a dependancy on a
> browser component...

Since this functionality is implemented in toolkit, browser/components/places looks irrelevant.
Bug 458849 has now/already been checked in on 1.9.1 branch :-|
Blocks: 458849
No longer blocks: CcMcBuildIssues
No longer depends on: 458849
Flags: wanted1.9.1?
OS: Windows 2000 → All
Hardware: x86 → All
Summary: [SeaMonkey] test_download_history.js fails now, with m-c/1.9.2 → [SeaMonkey] test_download_history.js fails now
Comment on attachment 354152 [details] [diff] [review]
patch v1

obsoleting, due to ehsan comment, seems like the current path for these tests is to try catch the getService, so we'll move there for now.
Attachment #354152 - Attachment is obsolete: true
Attachment #354152 - Flags: review?(sdwilsh)
Probably a good idea to leave it in toolkit but just not execute the pb part if the pb service isn't available.
And this doesn't test downloadmanager but places, right? (I'm just asking because SeaMonkey doesn't build toolkit DM for now - that's planned for the hopefully next few weeks though.)
(In reply to comment #11)
> And this doesn't test downloadmanager but places, right? (I'm just asking
> because SeaMonkey doesn't build toolkit DM for now - that's planned for the
> hopefully next few weeks though.)

Yes, I tried to correct myself in comment 5, but apparently the damage was already done!
Attached patch patch v1.1 (obsolete) — Splinter Review
this simply skips the pb test if pb is not available, like other tests.
Attachment #354818 - Flags: review?(dietrich)
Comment on attachment 354818 [details] [diff] [review]
patch v1.1

Nits only:

>+    // PrivateBrowsing component is not always available to Places implementers

I like comments with an ending point.

>+    var pb = Cc["@mozilla.org/privatebrowsing;1"].
>+           getService(Ci.nsIPrivateBrowsingService);

I prefer |var| before |try|.
Attached patch patch v1.2Splinter Review
fixed Serge's comments
Attachment #354818 - Attachment is obsolete: true
Attachment #354821 - Flags: review?(dietrich)
Attachment #354818 - Flags: review?(dietrich)
Comment on attachment 354821 [details] [diff] [review]
patch v1.2

>+  var pb = null;

Nit: |= null| isn't really needed in this case. (Yet, it does no harm.)
Attachment #354821 - Flags: review?(dietrich) → review+
http://hg.mozilla.org/mozilla-central/rev/a04c4267c6c3
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
verified1.9.1, as the tinderboxes are now passing this test.
Flags: wanted1.9.2?
Flags: wanted1.9.1?
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090110 SeaMonkey/2.0a3pre] (experimental/_m-c_, home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/6acaaa957e0a
 +http://hg.mozilla.org/comm-central/rev/865f907bb16b)

V.Fixed
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.