Closed Bug 1154294 Opened 5 years ago Closed 5 years ago

write a test for IE cookies migration

Categories

(Firefox :: Migration, defect)

defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 40
Iteration:
40.2 - 27 Apr
Tracking Status
firefox40 --- fixed

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file)

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+
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Iteration: --- → 40.2 - 27 Apr
Blocks: 1154472
Attached patch patch v1Splinter Review
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 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+
(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.
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → Firefox 40
https://hg.mozilla.org/mozilla-central/rev/2f8060bb3f94
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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.