Closed Bug 472631 Opened 11 years ago Closed 11 years ago

Private Browsing unit tests for password manager are not executed

Categories

(Toolkit :: Password Manager, defect, major)

defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: ehsan, Assigned: ehsan)

Details

(Keywords: verified1.9.1)

Attachments

(1 file)

Attached patch Patch (v1)Splinter Review
In <http://hg.mozilla.org/mozilla-central/rev/a6712f1f6c4e>, I failed to remove a few lines from test_privbrowsing.js, which causes the PB tests not to execute at all, because seemingly the PB service will be unavailable, which means that we've been running without any automated test coverage in password manager-related private browsing code, which is a shame.  :(

The fix is simple, and self-explanatory.
Attachment #355927 - Flags: review?(dolske)
Attachment #355927 - Flags: review?(dolske) → review+
Comment on attachment 355927 [details] [diff] [review]
Patch (v1)

*sadface*

Would it also be worth looking at making this more robust, by having the try/catch only ignore the specific exception that's thrown when the service doesn't exist? Or do we at least have a test in /browser somewhere that fails if that happens?
Oh, and do the tests pass with this fix, or does the problem in bug 472396 actually affect pwmgr too?
(In reply to comment #1)
> Would it also be worth looking at making this more robust, by having the
> try/catch only ignore the specific exception that's thrown when the service
> doesn't exist? Or do we at least have a test in /browser somewhere that fails
> if that happens?

We do have tests in browser/ which make sure the service exists and can be initialized.

(In reply to comment #2)
> Oh, and do the tests pass with this fix, or does the problem in bug 472396
> actually affect pwmgr too?

Yes, they do.  The problem in bug 472396 does not affect password manager, because it is itself implemented in JavaScript.




Landed on mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/02f596a6b80c>
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Attachment #355927 - Flags: approval1.9.1?
Comment on attachment 355927 [details] [diff] [review]
Patch (v1)

We *really* want this on 1.9.1, because without it we don't have test coverage on this part of the code at all.
Attachment #355927 - Flags: approval1.9.1?
Comment on attachment 355927 [details] [diff] [review]
Patch (v1)

This is just a test fix (no code), it should be fine to land on 1.9.1 without needing approval. a=me. :-)
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c141a6cc6ab5
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3
Target Milestone: mozilla1.9.1b3 → mozilla1.9.2a1
Seeing as there hasn't been any discussions about this bug for 3 months and it's just a test fix, I'm assuming there aren't any residual issues. I'm moving this to verified as a result. If anyone has any qualms, feel free to bring them up.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.