Code is { 86 // This test is Linux specific for now 87 if (osString != "Linux") { 88 finish(); 89 return; 90 } } I suggest to convert the comment to a |todo(false, "...");|.

why is that necessary?

(In reply to comment #1) > why is that necessary? See bug 492467 comment 1 and bug 483633 comment 3.

I skimmed through these bugs but I didn't find the explanation. Shouldn't the test harness be changed if there's an issue with failing tests not being detected in some circumstances?

(In reply to comment #3) > I skimmed through these bugs but I didn't find the explanation. Did you (not)? > Shouldn't the test harness be changed if there's an issue > with failing tests not being detected in some circumstances? This is precisely one thing we're getting fixed in (bug 492467 and) bug 483407, is it not?

(In reply to comment #4) > Did you (not)? So you want to check that tests have at least an assertion in order to detect tests that don't use waitForExplicitFinish() and fail without triggering the onerror() handler? > This is precisely one thing we're getting fixed in (bug 492467 and) bug 483407, > is it not? My point was that it would be better to fix the harness (one place) instead of all the tests that can have no assertions (lots of places) if possible. But I guess you would have done that if that was possible. By the way, would making waitForExplicitFinish() the default (i.e. requiring all tests to call finish()) be another way to solve this issue? Maybe that would make the silent failure detection even more robust (for instance when something bad happens after the first assertion and the onerror handler is not triggered for some reason). Moreover, that would make the tests more portable for browsers that don't implement onerror (WebKit).

(In reply to comment #5) > So you want to check that tests have at least an assertion in order to detect > tests that don't use waitForExplicitFinish() and fail without triggering the > onerror() handler? Yes, this is one case. > By the way, would making waitForExplicitFinish() the default (i.e. requiring > all tests to call finish()) be another way to solve this issue? Maybe; but that would not help tests which succeed but don't report (= count) anything. > Maybe that would make the silent failure detection even more robust (for > instance when something bad happens after the first assertion and the onerror > handler is not triggered for some reason). Moreover, that would make the tests > more portable for browsers that don't implement onerror (WebKit). You may file a bug about this if you want.

(In reply to comment #6) > (In reply to comment #5) > > So you want to check that tests have at least an assertion in order to detect > > tests that don't use waitForExplicitFinish() and fail without triggering the > > onerror() handler? > > Yes, this is one case. ok, do you happen to know the other cases off hand? > > By the way, would making waitForExplicitFinish() the default (i.e. requiring > > all tests to call finish()) be another way to solve this issue? > > Maybe; > but that would not help tests which succeed but don't report (= count) > anything. Why would it still be necessary to have successful tests report anything if they are required to call finish() in any case? (note that the answer to the previous question could answer this one too). If the test fails for some reason in the middle and the onerror handler is not called, finish() will never be called and the harness will be able to detect the failure. > > Maybe that would make the silent failure detection even more robust (for > > instance when something bad happens after the first assertion and the onerror > > handler is not triggered for some reason). Moreover, that would make the tests > > more portable for browsers that don't implement onerror (WebKit). > > You may file a bug about this if you want. Ok, this should probably be discussed on some mailing list before, as it can potentially require non trivial migration work (and it would be nice to have the requirement that tests have to always report something sorted out).

(In reply to comment #7) > (In reply to comment #6) > > Yes, this is one case. > > ok, do you happen to know the other cases off hand? > > > but that would not help tests which succeed but don't report (= count) > > anything. > > Why would it still be necessary to have successful tests report anything if > they are required to call finish() in any case? Even calling finish() doesn't mean the test actually checked anything: think of a code logic error or the like. And having a test _count_ as 0 (succeeded) check is just wrong in the first place. If all tests would be like that, it would trigger the "no tests/checks run" error! > (note that the answer to the previous question could answer this one too). > > If the test fails for some reason in the middle and the onerror handler is not > called, finish() will never be called and the harness will be able to detect > the failure. Finding failing tests is a (very nice) side-effect only: I'm interested in succeeding tests in the first place.

Ok, I think I see your points. What I can say from a test author point of view, is that having to put a dummy todo/ok when the tests has nothing to do (because it makes no sense for a platform, or because of some other conditions) is unintuitive and a bit ugly. But that shouldn't be a big deal (and the error message will make it clear if the author forgets to have at least an assertion I guess).

(In reply to comment #9) > What I can say from a test author point of view, is that having to put a dummy > todo/ok when the tests has nothing to do (because it makes no sense for a > platform, or because of some other conditions) is unintuitive and a bit ugly. Agreed, but that's not as unhelpful as you say: a todo(false,) reminds explicitly the test will need to be revisited later, an ok(true,) confirms explicitly the test "exited" where it is expected to. > But that shouldn't be a big deal (and the error message will make it clear if > the author forgets to have at least an assertion I guess). Yes, my basic concern is not about what the test checks, but about what feedback is given by the (test and) "harness" (in the log, etc).

Comment on attachment 429720 [details] [diff] [review] (Av1) Add a |todo(false, "...");| [Checkin: Comment 12 & 14] http://hg.mozilla.org/mozilla-central/rev/312285980801

V.Fixed, per http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1267621194.1267622666.9491.gz&fulltext=1

Comment on attachment 429720 [details] [diff] [review] (Av1) Add a |todo(false, "...");| [Checkin: Comment 12 & 14] http://hg.mozilla.org/releases/mozilla-1.9.2/rev/82cb6ada7eff