Re: WPT: PASS despite failed assertion

On 24/10/16 10:09 AM, Geoffrey Sneddon wrote:
> 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!

Thanks! I followed up to the pull request.

Received on Monday, 24 October 2016 22:21:06 UTC