760082 - toolkit/components/satchel/test/test_form_autocomplete_with_list.html ran additional tests after finish() was called

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Mats Palmgren (inactive)]

Fallout from the diagnostic patch in bug 677964.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Move SimpleTest.finish() after the last test is done. Use info() for log messages that aren't tests.

Comment 2

[Phil Ringnalda (:philor)]

Comment on attachment 628694 [details] [diff] [review]
Move SimpleTest.finish() after the last test is done.  Use info() for log messages that aren't tests.

That test lost me three times before I even got down to the patch.

Comment 3

[Justin Dolske [:Dolske]]

Comment on attachment 628694 [details] [diff] [review]
Move SimpleTest.finish() after the last test is done.  Use info() for log messages that aren't tests.

Fine with me if it fixes the problem. But... Are you sure this was the issue your diagnostic caught?

I'd have expected the input event to be fired asynchronously. And so the last bit of the case-400 should run before the input event fires. Is the event actually firing synchronously? I bet Mounir will know, his code and test. :)

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to Justin Dolske [:Dolske] from comment #3)
> Comment on attachment 628694 [details] [diff] [review]
> Move SimpleTest.finish() after the last test is done.  Use info() for log
> messages that aren't tests.
> 
> Fine with me if it fixes the problem. But... Are you sure this was the issue
> your diagnostic caught?
> 
> I'd have expected the input event to be fired asynchronously. And so the
> last bit of the case-400 should run before the input event fires. Is the
> event actually firing synchronously? I bet Mounir will know, his code and
> test. :)

This is actually not my code nor my test. That was David Zbarsky's. However, I can have a look.

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 628694 [details] [diff] [review]
Move SimpleTest.finish() after the last test is done.  Use info() for log messages that aren't tests.

Review of attachment 628694 [details] [diff] [review]:
-----------------------------------------------------------------

The best thing to do here to be on the safe side would be:
    case 400:
      // Check that the input event is fired.
      input.addEventListener("input", function(event) {
        input.removeEventListener("input", arguments.callee, false);
        ok(true, "oninput should have been received");
        ok(event.bubbles, "input event should bubble");
        ok(event.cancelable, "input event should be cancelable");

        checkForm("Google");
        SimpleTest.finish();
      }, false);

      doKey("down");
      checkForm("");
      doKey("return");

r=me with that.

Comment 6

[Mats Palmgren (inactive)]

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=5db9663b5473

Comment 7

[Mats Palmgren (inactive)]

OK, landed with that change:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac3642f73f24

Comment 8

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/ac3642f73f24