Closed
Bug 1154294
Opened 10 years ago
Closed 10 years ago
write a test for IE cookies migration
Categories
(Firefox :: Migration, defect)
Firefox
Migration
Tracking
()
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: mak, Assigned: mak)
References
Details
Attachments
(1 file)
10.20 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
We need a test, it should be possible to use Windows APIs through ctyped to create a cookie for migration.
Flags: qe-verify-
Flags: in-testsuite?
Flags: firefox-backlog+
Updated•10 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Iteration: --- → 40.2 - 27 Apr
Assignee | ||
Comment 1•10 years ago
|
||
Did a try run just to ensure there's no unexpected failure.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb153a24754c
Attachment #8592899 -
Flags: review?(gijskruitbosch+bugs)
Comment 2•10 years ago
|
||
Comment on attachment 8592899 [details] [diff] [review]
patch v1
Review of attachment 8592899 [details] [diff] [review]:
-----------------------------------------------------------------
This is pretty amazing. If you get to it before me, please feel free to land together with the date patch I stuck in the other bug. Minor niggles and suggestions below. :-)
::: browser/components/migration/tests/unit/test_IE_cookies.js
@@ +8,5 @@
> +
> + const BOOL = ctypes.bool;
> + const LPCTSTR = ctypes.char16_t.ptr;
> +
> + let wininet = ctypes.open("Wininet");
Can this throw and if so should we catch it and log a message so that if this ever breaks on try we have information? Say, if we start running this on win10 and for some reason it goes busted there? :-)
@@ +35,5 @@
> + const COOKIE = {
> + host: "mycookietest.com",
> + name: "testcookie",
> + value: "testvalue",
> + expiry
Wat. I didn't know this was possible in the new syntax, too. TIL.
@@ +45,5 @@
> +
> + // Create the persistent cookie in IE.
> + let value = COOKIE.name + " = " + COOKIE.value +"; expires = " +
> + COOKIE.expiry.toUTCString();
> + setIECookie(new URL("http://" + COOKIE.host).href, null, value);
So dumb question... what does your API call do if this cookie already exists and is that going to blow up if the test runs a second time on the same machine? I'm assuming you've tested this... :-)
Also, do I understand correctly that you declare this as returning a bool and then don't check the rv? Should we add a check for that just to be sure so that if this fails (returns false) we get that info and don't get inexplicable test failures? :-)
@@ +65,5 @@
> + Assert.equal(foundCookie.expiry, Math.floor(COOKIE.expiry / 1000));
> +});
> +
> +function run_test() {
> + run_next_test()
Is this necessary or something? Despite the add_task? That seems silly...
Attachment #8592899 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #2)
> > + let wininet = ctypes.open("Wininet");
>
> Can this throw and if so should we catch it and log a message so that if
> this ever breaks on try we have information? Say, if we start running this
> on win10 and for some reason it goes busted there? :-)
The test would fail then, and we'd get the usual exception with line number. I'm not sure it's useful to catch it preemptively.
> > + // Create the persistent cookie in IE.
> > + let value = COOKIE.name + " = " + COOKIE.value +"; expires = " +
> > + COOKIE.expiry.toUTCString();
> > + setIECookie(new URL("http://" + COOKIE.host).href, null, value);
>
> So dumb question... what does your API call do if this cookie already exists
> and is that going to blow up if the test runs a second time on the same
> machine? I'm assuming you've tested this... :-)
I actually assumed the Windows API would overwrite it silently, as it would happen for a cookie set by the browser. I will test that, but I think any other behavior would not make sense.
> Also, do I understand correctly that you declare this as returning a bool
> and then don't check the rv? Should we add a check for that just to be sure
> so that if this fails (returns false) we get that info and don't get
> inexplicable test failures? :-)
Yep, I forgot to check the rv. It would still fail, but better to be explicit.
> @@ +65,5 @@
> > + Assert.equal(foundCookie.expiry, Math.floor(COOKIE.expiry / 1000));
> > +});
> > +
> > +function run_test() {
> > + run_next_test()
>
> Is this necessary or something? Despite the add_task? That seems silly...
I didn't know bug 982852 was finally fixed 10 days ago... Good to know.
Assignee | ||
Updated•10 years ago
|
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → Firefox 40
Comment 5•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Comment 6•10 years ago
|
||
Nice work, thanks for getting this test in Marco!
You need to log in
before you can comment on or make changes to this bug.
Description
•