Re: WPT: PASS despite failed assertion

On 23/10/16 21:29, Jack Bates wrote:
> On 20/10/16 02:33 PM, James Graham wrote:
>> On 20/10/16 22:13, Geoffrey Sneddon wrote:
>>> On Sat, Oct 15, 2016 at 4:30 PM, Jack Bates <z1a8te@nottheoilrig.com>
>>> wrote:
>>>> I noticed a couple of tests that pass even if one of their assertions
>>>> fails.
>>>> To see what I mean, add an assert_true(false) to
>>>> fetch/api/cors/cors-preflight-redirect.js, for one.
>>>> My browser logs the AssertionError, but testharness.js doesn't catch
>>>> it, so
>>>> the test passes despite the failed assertion.
>>>> Is this a known issue? Can testharness.js be improved so the failed
>>>> assertion doesn't just vanish?
>>>> I think the issue can be addressed superficially in the tests
>>>> themselves,
>>>> but at a higher level, is there something testharness.js can do to
>>>> avoid
>>>> concealing the issue by swallowing the failed assertion?
>>>
>>> Where in fetch/api/cors/cors-preflight-redirect.js? Asserts are only
>>> picked up within tests (with some magic for single-file tests where
>>> the whole file is considered tests), hence only asserts within the
>>> anonymous function provided as the first argument to promise_test will
>>> get picked up. (Hopefully that should always get picked up; I don't
>>> know well how our support for promises works.)
>>>
>>> One approach could be to have an error event listener on the document
>>> that sets the whole test (i.e., all subtests) into some error state if
>>> there were an (uncaught) assert reach it. Anyone else have any
>>> opinion?
>>
>> There is an error listener that sets the overall file status to error if
>> there is an uncaught exception. I think this is reasonable (and any
>> other change would be backwards-incompatible), since an exception is
>> either a bug in the test or a fundamental problem with the browser under
>> test (e.g. it doesn't support the APIs being tested).
> 
> I see what you mean [1] (addEventListener("error" ...)
> What about expanding this to cover unhandled assertions located in
> promise handlers? Something like the following:
> 
>>          }
>>      });
>>
>> +    addEventListener("unhandledrejection", function(event) {
>> +        var e = event.reason;
>> +        if (tests.file_is_test) {
>> +            var test = tests.tests[0];
>> +            if (test.phase >= test.phases.HAS_RESULT) {
>> +                return;
>> +            }
>> +            test.set_status(test.FAIL, e.message, e.stack);
>> +            test.phase = test.phases.HAS_RESULT;
>> +            test.done();
>> +            done();
>> +        } else if (!tests.allow_uncaught_exception) {
>> +            tests.status.status = tests.status.ERROR;
>> +            tests.status.message = e.message;
>> +            tests.status.stack = e.stack;
>> +        }
>> +    });
>> +
>>      test_environment.on_tests_ready();
>>
>>  })();
> 
> To see what I mean about existing tests whose outcome is currently
> invisible, see what happens if one of the assertions in
> fetch/api/cors/cors-redirect.js fails, like so:
> 
>>    return promise_test(function(test) {
>>      fetch(RESOURCES_DIR + "clean-stash.py?token=" +
>> uuid_token).then(function(resp) {
>>        return fetch(url + urlParameters,
>> requestInit).then(function(resp) {
>> -        assert_equals(resp.status, 200, "Response's status is 200");
>> +        assert_true(false);
>>          assert_equals(resp.headers.get("x-did-preflight"), "0", "No
>> preflight request has been made");
>>          assert_equals(resp.headers.get("x-origin"), expectedOrigin,
>> "Origin is correctly set after redirect");
>>        });
> 
> I expect the test to fail, but it passes.
> I think this can be addressed superficially in each test, but is there
> something testharness.js can do to avoid concealing this?
> 
> [1] https://github.com/w3c/testharness.js/blob/master/testharness.js#L2668

So I think <https://github.com/w3c/testharness.js/pull/219> resolves
this. (I'd forgotten uncaught errors in promises only caused the
unhandled rejection event and not also error!) If it doesn't, please
comment there!

/gsnedders

Received on Monday, 24 October 2016 17:09:57 UTC