Re: WPT: PASS despite failed assertion

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

Received on Sunday, 23 October 2016 20:29:50 UTC