659522 - tests in window_activation.xul don't test anything (and when they're fixed, 2 of them fail)

Status: NEW

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

The 4 tests inside of window_activation.xul (which is part of test_activation.xul) don't actually test anything right now.  e.g.:

> ok(getComputedStyle(document.getElementById("box"), "").backgroundColor, "rgb(0, 0, 255)");

These "ok" calls really want to be "is" calls.  Currently, the ok() test just checks if the first argument is "truthy", which doesn't tell us anything.

Sadly, when I replace the "ok()" checks with "is()" checks, 2 of the checks fail. :(  I'm guessing that this has been the case since the test was added in the first place.  I don't know if that's indicative of a code bug or a test bug, but either way, we should fix it.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: first patch: replace "ok" with is & todo_is

Comment 2

[Daniel Holbert [:dholbert]]

I'm posting this first patch until we have an actual fix here, so that we'll be checking _something_ rather than not checking anything. :)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: first patch v2: replace "ok" with is & todo_is (has issues on TryServer)

Thanks!

Here's the patch again, with a typo fix in one of the test-failure-messages.  (s/blue/cyan/) & with r+ noted.

I'm currently running the previous (functionally-equivalent) version of patch through TryServer just to be sure the is() & todo_is() conditions behave the same on our tinderboxen as on my local machine.

I'm also making a targeted build from the changeset that added this test, to see if our behavior was the same back then.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to comment #3)
> I'm also making a targeted build from the changeset that added this test, to
> see if our behavior was the same back then.

Targeted build[1] done - I've confirmed that our behavior was the same back then.  With my patch here, the testcase's first two checks are satisfied, and the second two are not (and hence need todo_is to prevent permaorange).

[1] at http://hg.mozilla.org/mozilla-central/rev/e17c076aceea

Comment 5

[Markus Stange [:mstange]]

(In reply to comment #0)
> These "ok" calls really want to be "is" calls.

Oops. How did you catch this!?

I've sent a patch that should fix the whole test to the tryserver:
http://tbpl.mozilla.org/?tree=Try&rev=60918e5a12e2

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to comment #5)
> Oops. How did you catch this!?

Over in bug 659466. :)

You'll want to base your work here off of that bug's patch, in order for your tests' stylesheets to be correctly parsed.

Comment 7

[Daniel Holbert [:dholbert]]

BTW, tinderbox doesn't like my patch here -- it looks like the test boxes behave differently from my local box for some reason (and mac/windows test boxes behave differently from linux).

My try push (including the data URI fixup from bug 659466):
  http://tbpl.mozilla.org/?tree=Try&rev=09799053faba

That has these unexpecteds on linux:
ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_activation.xul | checking that background is initially cyan in openerDoc - got "rgb(0, 0, 255)", expected "rgb(0, 255, 255)"
ERROR TEST-UNEXPECTED-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_activation.xul | checking that background is blue in openerDoc after it's focused - "rgb(0, 0, 255)" should equal "rgb(0, 0, 255)"

...with this additional unexpected on Windows/Mac:
ERROR TEST-UNEXPECTED-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_activation.xul | checking that background is cyan in document after it's unfocused - "rgb(0, 255, 255)" should equal "rgb(0, 255, 255)"

So my patch here shouldn't land in its current state. Markus, I fully support you improving upon it & landing something better. :)