Testing that our exceptions have good messages

[Jeffrey Yasskin]

I sent https://critic.hoppipolla.co.uk/r/5622 to give testharness.js's assert_throws() the ability to check error messages, and got the objection back that this is counter to the goal of sharing tests between implementations.

So, how do we want to test our error messages? Writing a completely parallel set of error tests seems wasteful, but ms2ger's right that it'll be hard to write a single test that lets implementations iterate on their messages.

Thoughts?

Jeffrey

[Matt Falkenhagen]

For Service Worker we somewhat do the "parallel set of tests" thing. Tests in http/tests/serviceworker/chromium/, which won't be upstreamed to Web Platform Tests, include some error message tests.

I agree it's not so great. I wonder if testharness.js can include something like output(e.error) which is just ignored when run with WPT test runner but Blink test runner would output the string and compare against an expectations file. 

 

[Jeffrey Yasskin]

Having assert_throws() call output(e.message) will force us to update every expectations file at once, and will force us to test the exact spelling of the error message, which is probably too precise in many situations. Having a separate function to print the message will remove the conciseness of assert_promise_rejects() calls, since we'll have to extract the error in a separate line. Even giving assert_throws() a parameter to optionally print the message will still force users to depend on exact error message spellings, which is often a recipe for fragile tests. That's why my patch allowed regular expressions, not just exact strings.

That said, maybe the opt-in route that checks exact messages is the best alternative. Are expectations files not part of the upstream tests?

Jeffrey

[Dirk Pranke]

On Wed, Jul 22, 2015 at 4:43 PM, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

Having assert_throws() call output(e.message) will force us to update every expectations file at once, and will force us to test the exact spelling of the error message, which is probably too precise in many situations. Having a separate function to print the message will remove the conciseness of assert_promise_rejects() calls, since we'll have to extract the error in a separate line. Even giving assert_throws() a parameter to optionally print the message will still force users to depend on exact error message spellings, which is often a recipe for fragile tests. That's why my patch allowed regular expressions, not just exact strings.

That said, maybe the opt-in route that checks exact messages is the best alternative. Are expectations files not part of the upstream tests?

Correct, upstream tests do not have expectations per se. Either the tests are manual (== pixel tests), ref tests, or assertion-based, in which case they only care about what assertions might fail. We do a bit more and generate expectations when we want to track a test that only fails partially, but do not require expectations for most tests.

-- Dirk 

[Jeffrey Yasskin]

On Wed, Jul 22, 2015 at 5:28 PM, Dirk Pranke <dpr...@chromium.org> wrote:

On Wed, Jul 22, 2015 at 4:43 PM, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

Having assert_throws() call output(e.message) will force us to update every expectations file at once, and will force us to test the exact spelling of the error message, which is probably too precise in many situations. Having a separate function to print the message will remove the conciseness of assert_promise_rejects() calls, since we'll have to extract the error in a separate line. Even giving assert_throws() a parameter to optionally print the message will still force users to depend on exact error message spellings, which is often a recipe for fragile tests. That's why my patch allowed regular expressions, not just exact strings.

That said, maybe the opt-in route that checks exact messages is the best alternative. Are expectations files not part of the upstream tests?

Correct, upstream tests do not have expectations per se. Either the tests are manual (== pixel tests), ref tests, or assertion-based, in which case they only care about what assertions might fail. We do a bit more and generate expectations when we want to track a test that only fails partially, but do not require expectations for most tests.

So is this the direction we want to go? Have assert_throws(func, {name: 'FooError', printMessage: true}) call printErrorMessage(e.message), which is provided by testharnessreport.js?

[Dirk Pranke]

On Wed, Jul 22, 2015 at 5:36 PM, Jeffrey Yasskin <jyas...@google.com> wrote:

On Wed, Jul 22, 2015 at 5:28 PM, Dirk Pranke <dpr...@chromium.org> wrote:

On Wed, Jul 22, 2015 at 4:43 PM, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

Having assert_throws() call output(e.message) will force us to update every expectations file at once, and will force us to test the exact spelling of the error message, which is probably too precise in many situations. Having a separate function to print the message will remove the conciseness of assert_promise_rejects() calls, since we'll have to extract the error in a separate line. Even giving assert_throws() a parameter to optionally print the message will still force users to depend on exact error message spellings, which is often a recipe for fragile tests. That's why my patch allowed regular expressions, not just exact strings.

That said, maybe the opt-in route that checks exact messages is the best alternative. Are expectations files not part of the upstream tests?

Correct, upstream tests do not have expectations per se. Either the tests are manual (== pixel tests), ref tests, or assertion-based, in which case they only care about what assertions might fail. We do a bit more and generate expectations when we want to track a test that only fails partially, but do not require expectations for most tests.

So is this the direction we want to go? Have assert_throws(func, {name: 'FooError', printMessage: true}) call printErrorMessage(e.message), which is provided by testharnessreport.js?

To be clear: I am not endorsing a solution here (I haven't really thought about it yet).

-- Dirk

[Joshua Bell]

On Wed, Jul 22, 2015 at 5:36 PM, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

On Wed, Jul 22, 2015 at 5:28 PM, Dirk Pranke <dpr...@chromium.org> wrote:

On Wed, Jul 22, 2015 at 4:43 PM, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

Having assert_throws() call output(e.message) will force us to update every expectations file at once, and will force us to test the exact spelling of the error message, which is probably too precise in many situations. Having a separate function to print the message will remove the conciseness of assert_promise_rejects() calls, since we'll have to extract the error in a separate line. Even giving assert_throws() a parameter to optionally print the message will still force users to depend on exact error message spellings, which is often a recipe for fragile tests. That's why my patch allowed regular expressions, not just exact strings.

That said, maybe the opt-in route that checks exact messages is the best alternative. Are expectations files not part of the upstream tests?

Correct, upstream tests do not have expectations per se. Either the tests are manual (== pixel tests), ref tests, or assertion-based, in which case they only care about what assertions might fail. We do a bit more and generate expectations when we want to track a test that only fails partially, but do not require expectations for most tests.

So is this the direction we want to go? Have assert_throws(func, {name: 'FooError', printMessage: true}) call printErrorMessage(e.message), which is provided by testharnessreport.js?

IMHO, that sounds like a plausible direction to run past the testharness maintainers to see if they're amenable or have a better suggestion - my thinking was leading in the same direction. We'd probably need to change our PRESUBMIT (etc) that currently rejects an -expected.txt that contains only PASS and no FAIL lines to allow e.g. PASS+MESSAGE. It assumes test maintainers can be convinced to sprinkle the primtMessage: flag in appropriate tests.

This probably deserves a thread on public-test-infra.

(On tangent, I should note that it's explicitly a non-goal of the testharness maintainers to have stable error messages from failing assertions. For example, they consider it acceptable for an assertion message to contain a timestamp or random number, or platform specific data, which would not work with our -expected.txt mechanism. That *should* be orthogonal to your proposal above since the called function would be implemented by us and we'd explicitly want the input to be stable and fail if the message changes.)

[ja...@hoppipolla.co.uk]

On Thursday, 23 July 2015 01:51:49 UTC+1, Joshua Bell wrote:

On Wed, Jul 22, 2015 at 5:36 PM, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

So is this the direction we want to go? Have assert_throws(func, {name: 'FooError', printMessage: true}) call printErrorMessage(e.message), which is provided by testharnessreport.js?

IMHO, that sounds like a plausible direction to run past the testharness maintainers to see if they're amenable or have a better suggestion - my thinking was leading in the same direction. We'd probably need to change our PRESUBMIT (etc) that currently rejects an -expected.txt that contains only PASS and no FAIL lines to allow e.g. PASS+MESSAGE. It assumes test maintainers can be convinced to sprinkle the primtMessage: flag in appropriate tests.

This probably deserves a thread on public-test-infra.

Yes, please start a thread on public-test-infra about this issue.

My initial reaction to your proposal is that it still leaves blink-specific data in the tests because the specific set of messages that are marked for printing has to exactly match the set the blink developers want to check. Therefore I don't think it will work. It's also possible that the order of assertions in tests — in particular async tests — is non-deterministic, so I see a lot of potential for flakiness if you are just matching messages in the order you receive them.

I think having extra data from a test that can be used to perform UA-specific, result changing, steps is probably possible but it will require some careful consideration and, honestly, I'm not sure it won't turn out to be more trouble than its worth. But let's work through it on public-test-infra.

[Jeffrey Yasskin]

On Wed, Jul 22, 2015 at 5:51 PM, Joshua Bell <jsb...@chromium.org> wrote:

On Wed, Jul 22, 2015 at 5:36 PM, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

On Wed, Jul 22, 2015 at 5:28 PM, Dirk Pranke <dpr...@chromium.org> wrote:

On Wed, Jul 22, 2015 at 4:43 PM, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

Having assert_throws() call output(e.message) will force us to update every expectations file at once, and will force us to test the exact spelling of the error message, which is probably too precise in many situations. Having a separate function to print the message will remove the conciseness of assert_promise_rejects() calls, since we'll have to extract the error in a separate line. Even giving assert_throws() a parameter to optionally print the message will still force users to depend on exact error message spellings, which is often a recipe for fragile tests. That's why my patch allowed regular expressions, not just exact strings.

That said, maybe the opt-in route that checks exact messages is the best alternative. Are expectations files not part of the upstream tests?

Correct, upstream tests do not have expectations per se. Either the tests are manual (== pixel tests), ref tests, or assertion-based, in which case they only care about what assertions might fail. We do a bit more and generate expectations when we want to track a test that only fails partially, but do not require expectations for most tests.

So is this the direction we want to go? Have assert_throws(func, {name: 'FooError', printMessage: true}) call printErrorMessage(e.message), which is provided by testharnessreport.js?

IMHO, that sounds like a plausible direction to run past the testharness maintainers to see if they're amenable or have a better suggestion - my thinking was leading in the same direction. We'd probably need to change our PRESUBMIT (etc) that currently rejects an -expected.txt that contains only PASS and no FAIL lines to allow e.g. PASS+MESSAGE. It assumes test maintainers can be convinced to sprinkle the primtMessage: flag in appropriate tests.

This probably deserves a thread on public-test-infra.

(On tangent, I should note that it's explicitly a non-goal of the testharness maintainers to have stable error messages from failing assertions. For example, they consider it acceptable for an assertion message to contain a timestamp or random number, or platform specific data, which would not work with our -expected.txt mechanism. That *should* be orthogonal to your proposal above since the called function would be implemented by us and we'd explicitly want the input to be stable and fail if the message changes.)

The rough proposal from https://lists.w3.org/Archives/Public/public-test-infra/2015JulSep/0002.html and IRC is now that we'd add some sort of test.add_data(name, data) to testharness.js, which testharnessreport.js would interpret. The simplest way to interpret it in Blink would be to sort by |name| and dump it to be compared with a block at the end of the -expected.txt file, but James points out that this may bite us when someone else adds data that's not stable across Chrome platforms. We may wind up needing to load data from the test context instead of just comparing for equality.

Is Blink ok with that? Would we prefer some other solution?

Thanks,

Jeffrey

[James Graham]

On 29/07/15 00:47, 'Jeffrey Yasskin' via blink-dev wrote:

> The rough proposal from
> https://lists.w3.org/Archives/Public/public-test-infra/2015JulSep/0002.html
> and IRC is now that we'd add some sort of test.add_data(name, data) to
> testharness.js, which testharnessreport.js would interpret. The simplest
> way to interpret it in Blink would be to sort by |name| and dump it to
> be compared with a block at the end of the -expected.txt file, but James
> points out that this may bite us when someone else adds data that's not
> stable across Chrome platforms. We may wind up needing to load data from
> the test context instead of just comparing for equality.

Specifically I would worry about the case where someone else adds some
data here that is constant in their implementation but variable in the
blink implementation because e.g. it contains a timestamp. If you don't
have a mechanism to whitelist the tests for which you use this
additional comparison data I would be very wary of trying to use it
anywhere, because anything that makes it harder and more manual to
import tests is a big impediment to running the whole suite, which I
hope is a goal.