RFC #16 - PRECONDITION_FAILED subtest result (status)

[lukebjerring]

Rendered

[jgraham]

I'm somewhat on the fence about whether this is a good idea. In practice truely optional features are rare and almost always represent compat hazards. We don't want to go out of our way to encouage such optionality.

I am concerned about the amount of code that has to change to support this. For example mozlog has to change, and log consumers may also have to change to handle the status in a meaningful way. Of course it's not possible to enumerate the full set of log consumers.

In terms of the API, this isn't an assert, it shold be a method on the test like force_timeout which immediately sets the status to TIMEOUT.

[jugglinmike]

The RFC currently focuses on consumers of test results. You may also want to note that this makes the tests themselves more expressive, allowing authors to be more explicit about their expectations.

Currently, tests for proposed specification changes are denoted with a filename pattern (.tentative.). This use case seems semantically equivalent, and since your solution operates at a sub-test level, it's more precise. I'm wondering if your solution could replace the file name convention. That would reduce the learning curve for contributors.

One nice aspect of implementing this as an assertion is that "failures" will interrupt further testing. Maybe invert the meaning, though. assert_implemented wouldn't need to be used from a separate codepath; the condition could be expressed as an argument to the function.

[lukebjerring]

In practice truely optional features are rare and almost always represent compat hazards

While rare (I'm only currently aware of Codecs, and WebCryptoAPI), I still feel like this is an important distinction to make, especially for permanently unimplemented features.

WebCryptoAPI, for example, could have a valid implementation that didn't implement any of the encryption algorithms. (IMHO, it's preferable that we achieve interop for the implementations, since this would avoid a web dev needing to worry about which user agents implement which algorithms). We've heard back from Chrome that not supporting AES 192 bit, for example, is working as intended.

The conclusion was that Web Crypto does not mandate support for any particular algorithms, and would only go so far as to recommend certain algorithms at the time it was published. Compatibility has been left to external interop profiles.

Thus, the "FAIL" WebCryptoAPI results aren't really useful as failures (and create unnecessary noise).

[lukebjerring]

@jugglinmike - nice to hear two sides of the argument. Under further scrutiny, though, I think I side with @jgraham here in that we don't want to encourage people to unnecessarily make spec features optional, so I'm unsure about coupling this change with .tentative. use-cases.

As I understand, .tentative. is more around whether the details covered by the test will actually stick to the spec long term. My intention for NOT_IMPLEMENTED is more around the case of flagging tests as "if this, of many, optional implementation does exists, here's some assertions about its behaviour".

Bailing out (immediately) as a forced test result via test.unimplemented() is equally as expressive, since the conditions leading up to it can be try/catch, if, whatever.

[lukebjerring]

@foolip - was codecs the only other example you mentioned? Are you aware of other APIs that list optional capabilities?

[foolip]

@lukebjerring will be OOO for a while, so I suggest we put this on ice until he's back.

One option that we could consider is something inspired by what mocha does. There, the context object, which is like our test object, has a skip method. It'd work like this:

test(t => {
  if (thingIsntSupported) {
    t.skip('maybe a reason here'));
  }
}, 'test name');

This aligns with @jgraham's suggestion to use a method and test.unimplemented(), but is a bit more explicit. It would still result in a new subtest status however.

[foolip]

@lukebjerring now that you're back, do you want to pick this up again? What do you think about the t.skip() method to match mocha?

[Orphis]

I am currently writing codec related tests for the WebRTC test suite and this feature (or something equivalent) would definitely be useful to convey a better meaning to the test results.

I do prefer the t.skip() approach better in this case.

[foolip]

@lukebjerring https://wpt.fyi/results/html/semantics/embedded-content/media-elements/mime-types/canPlayType.html is the media element codec test I had in mind. It adds "(optional)" to some test names, but unfortunately I don't know that this test would be much improved by a mechanism to skip subtests, as the "is this implemented" check itself is what the test is for.

[lukebjerring]

I agree with the ergonomics of a t.skip. Let's update the RFC to reflect that.

[Ms2ger]

Is it intentional that the execution continues after the skip() call in this example?

[foolip]

As a point of reference, mocha does throw in its this.skip():
https://github.com/mochajs/mocha/blob/186ca3657b4d3e0c0a602a500653a695f4e08930/lib/runnable.js#L138

We could do that with a specific error type, or we could treat this as t.done() and keep going but ignore anything the test does. The difference of course is that it's unlikely there's stuff after t.done() but very likely there's stuff after t.skip().

@lukebjerring WDYT?

[lukebjerring]

Not intentional (prototype was tweaked to fix that).

@foolip I would expect t.skip to be the "end of the road" in so far as the test content is concerned, since "ignoring" further execution would be more computationally expensive, and extra code for the testharness.

[Ms2ger]

Presumably this would be a DOMException and this example code would be broken.

[lukebjerring]

You're correct. I made some assumptions after skimreading the spec, but on closer inspection, it's a DOMException with no strongly identifiable information :/

[foolip]

@lukebjerring I'm writing a test today that could make use of this. Is there anything I can review to help move this along?

[jgraham]

I commented in the related PR, but I'm somewhat concerned that the approach of reusing SKIP here makes it impossible to tell the difference between a test that has been explicitly disabled (e.g. due to instability) and one that has failed a precondition check in the test itself. The ability to audit tests that have been disabled is useful and I'm nervous about regressing that.

[foolip]

When disabling tests, isn't the status of the whole test SKIP, rather than of the subtests?

[jgraham]

You can also disable individual subtests (basically meaning that they run but the result is replaced by SKIP).

[lukebjerring]

If in understand correctly, there's no overlap between usage of the two features (in-test skip and disable-skip); only a common outcome. There's still a distinguishing message field for the reason they were skipped, and I'd expect you would never disable a skipping subtest as unstable (though I guess we could contrive a badly written test that makes it unstably skip sometimes).

Can you give an example of how the feature is used (or point to some docs)? I wasn't aware of it.

[jgraham]

See e.g. https://searchfox.org/mozilla-central/source/testing/web-platform/meta/html/dom/interfaces.https.html.ini#61 which disables that subtest. Actually looking at the code it seems we are just ignoring those tests (see https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/wptrunner/testrunner.py#561-562); that's clearly a bug and we should set the status to SKIP like we do for top-level tests.

[lukebjerring]

So, we actually started with NOT_IMPLEMENTED in the proposal, but was too specific, since non-implementation isn't the only reason we might skip. Funnily enough, the use of SKIP you describe might be better named as DISABLED?

Is there a way of 'overloading' result types? Like, multiple enum values that use the same numeric value?

[jgraham]

I don't think we can overload result types. Changing SKIP to DISABLED unfotunately breaks any tooling that currently depends on SKIP (the fact that it isn't provided on subtests not withstanding; tools can often treat subtest statuses and test statuses identically). It seems like the cases discussed here are more like PRECONDITION_FAILED, if the main concern is naming.

[lukebjerring]

Naming is only a concern once disambiguation is made a prerequisite. It seems that your need for distinguishing "infra skipped" vs "test skipped" is only theoretical, given that the implementation as-is is omitting the data entirely (which wpt.fyi would show as MISSING, a cosmetic/UI fake-status).

Another approach, which I understand has come up before, is to support multiple expectations. This is Chromium's approach to ignoring flakiness, and the equivalent here would just be support an ANY alias that lets all statuses through. I believe @LukeZielinski has this work on his radar, as it relates to using the wpt-runner directly inside Chromium's CQ.

What are your thoughts on multi-expectations?

[jgraham]

There is separately work on mutliple expectations happening. But I don't expect it to be a full replacement for disabling tests, so having SKIP either mean "disabled" or "test decided not to run" is still confusing.

[lukebjerring]

There is separately work on mutliple expectations happening

Is there an RFC/PR/bugzilla entry for tracking the work? And yeah that's true, there's still use-cases for disabling tests outside of flakiness, so it won't be a full replacement.

In those remaining cases, though, as it stands the behaviour around SKIP for disabled tests is misleading; the test wasn't skipped (it did run) and the result is being omitted/ignored. In those cases, there's a need to disambiguate an omitted (MISSING) result as "dropped because ignored" vs "not actually run", and use of the SKIP result is probably a misnomer.

Given that (due to what you call a bug in the implementation) we're not actually in a position where SKIP subtests is ambiguous, can we address the need to distinguish "disabled" separately? Perhaps in another RFC? My first thoughts are that DISABLED is a better status name for those cases, and we could simply migrate that codepath (which currently ignores the subtest entirely) to emit a different status eventually, having a slight risk of confusion in the short-term to unblock the need for SKIP motivating this RFC.

[jgraham]

So it seems like this RFC needs to be updated to match the decisions that have been made, but once that's done it should be possible to go ahead and merge it.

[lukebjerring]

I've updated to reflect the consensus (as I understand it to be). PTAL.

[foolip]

Renamed file to assert_precondition.md to match the name of the added API surface.

[foolip]

Since the proposal changed let's leave a little bit more time for people to react, but I think it'd be low risk to start implementing this now across the repos impacted.

[Ms2ger]

How about

            assert_precondition(!isUnsupported(err), algorithm + " not implemented");
            assert_unreached("Threw an unexpected error: " + err.toString());

[foolip]

I like collapsing as well, but then we can invert the naming to avoid the negation.

[jgraham]

To be clear, you're suggesting

            assert_precondition(isSupported(err), algorithm + " not implemented");
            assert_unreached("Threw an unexpected error: " + err.toString());

? It took me a while to be confident of which part of the naming you wanted to invert.

[foolip]

Yes, isUnsupported to isSupported.

[lukebjerring]

isUnsupported(err) makes more sense as a function name ("is this error telling me the feature is unsupported") but supported makes more sense as the param name for the assert... so I made it 3 lines to meet you halfway.

[foolip]

Still agree with @Ms2ger on this one. Most feature detects will be inlined and be "positive" expressions like SharedWorker in self or navigator.xr.

[jgraham]

Isn't this bikeshedding over an example that doesn't affect the implementation? Or am I missing some reason it's very important to be perfect here?

[lukebjerring]

Right, but isSupported(err) checks that the error isn't unsupported.. which doesn't make sense.

🚲 And yes, bikeshedding for sure 🚲

[Ms2ger]

Fwiw, I was more concerned about the if (foo) assert(false) pattern than inlining the condition.

[foolip]

I was bikeshedding as if this were the documentation and an example people will copy. Happy to leave that scrutiny to the implementation PR and then come back here and update to match how the description needs to be phrased.

Let's merge this and move on to the PR on wpt.

[foolip]

I realize there's an issue with descriptions that often comes up. Most of the assert methods generate failure message that assume the description describes the actual value or what did happen. But it's somewhat common for people to write descriptions saying what the value shouldn't be or what shouldn't happen. That leads to pretty bad failure messages.

How will the failure message be constructed for this? The prefix "Precondition not met: " would require a positive description of what is (should) be supported, but is that the format you had in mind?

Since it's so easy to get this wrong, a clear example would be great. It'll still be wrong 30% of the time though :)

[foolip]

All who participated in this RFC, a review is now up at web-platform-tests/wpt#19993 and I think there are some details worth a second opinion on. I'll add comments on those bits and see if I get any feedback :)