Tracking issue: issues with testing and reporting workerd results

[jasnell]

I'm going to use this issue to track a number of issues with the reporting of results for workerd. Will add issues as the cause is determined...

1. URL conformance... The results currently show workerd as not implemented URL. This is incorrect, workerd provides a fully WHATWG compliant URL implementation. It appears the issue causing workerd to fail these tests is that the tests themselves use location.href, which workerd does not implement. Specifically, tests.json includes a number of tests using the pattern:

  "api.URL.host": {
    "code": "(function () {\n  var instance;\n  try {\n    instance = new URL(location.href);\n  } catch (e) {\n    instance = new webkitURL(location.href);\n  }\n  return !!instance && \"host\" in instance;\n})();\n",

Either a different input URL should be used for the test harness in generator/runtimes/workerd/handler.ts should be updated to polyfill location.href

2. The test results check for AbortSignal.prototype.onabort. Workerd definitely does not define the onabort property on the AbortSignal prototype but it does, in fact support use of signal.onabort = .... This is a discrepancy with how workerd supports the spec but given that the method does work, the result table is inaccurate. Implement AbortSignal.protototype.onabort cloudflare/workerd#1848 PR fix for workerd has landed. It is not yet in production. Should be relatively soon tho.

3. The test results indicate that workerd does not support the ByteLengthQueuingStrategy. This might be a discrepancy in how the class is exposed on our global scope vs. how the test looks for it. self.ByteLengthQueuingStrategy works just fine and the constructor is supported. I suspect that the same applies to the CompressionStream constructor, CountQueuingStrategy constructor, CustomEvent constructor, etc. This is actually an issue with the test harness. Currently it tests for the presence of a class constructor by trying to call the constructor with no arguments. If it works, cool. If it throws, it determines support vs non-support by checking the error message. Unfortunately the harness currently does not account for the error messages provided by workerd so it flags those as failures. Update constructor harness to support workerd #119

4. The test results indicate that workerd does not support the CloseEvent. It actually does but we have a bug. The test tries to create the CloseEvent using only a single string argument in the constructor, eliding the optional second options argument. Looks like workerd is incorrectly set to require that second argument right now so the test fails trying to construct the instance. I will open an issue to fix that, in the meantime tho, the rest of it is supported. The second argument to CloseEvent is supposed to be optional cloudflare/workerd#1849

5. The test results indicate that the new Headers getSetCookie() API is not supported. This is false. Unfortunately for backwards compatibility reasons we require a compatibility flag to be set (https://developers.cloudflare.com/workers/configuration/compatibility-dates/#headers-supports-getsetcookie) to enable the feature but it is on by default as of a year ago. Update the workerd compatibility date #79

6. For the PromiseRejectionEvent, we definitely do not support the constructor but we do support the promise and reason properties.

7. For the ReadableByteStreamController desiredSize property. This is a discrepancy with how workers exposes the property. The test looks for it on the prototype, while the runtime exposes it as an instance property. This is probably a bug in workerd that I need to look at but the property itself is supported. Likewise with the view property on ReadableStreamByobRequest Convert the standard properties on streams objects to prototype cloudflare/workerd#1850 PR fix for workerd has landed. It is not yet in production. Should be relatively soon tho.

8. TransformStreamDefaultController is fully supported in workerd.

9. We have a PR to improve TextEncodingStream and TextDecoderStream conformance by adding the missing properties - Add standard properties to TextDecoderStream/TextEncoderStream cloudflare/workerd#1981 PR landed.

10. We have a PR to add reportError(...) and add the missing ErrorEvent constructor - Implements the web platform standard reportError API cloudflare/workerd#1979 PR landed.

I imagine that it will likely take a few pull requests (as well as a couple of modifications in workerd) to update the compat table for these cases.

[ascorbic]

For 5, is this an option that can be set in our workerd runner?

[ascorbic]

I don't think polyfilling location.href will be enough, because the Request tests all try to instantiate it with an empty string, which also doesn't work. We'll need to see if we can fix the tests.

[jasnell]

@ascorbic ... for #5, yes, it would be set as a compat flag in the worker config. Honestly I think it should be the default already tho, I need to look into it a bit more....

Update: ah, yes, your compatibilityDate is set to 2023-02-28 which means the getSetCookie() flag won't be on by default. If you bump that up to some time in 2024 then you should pick up that change.

[ascorbic]

Ah ok. If you'd like to do a PR to update it to the best settings to represent what users would get then that would be great.

[jasnell]

Ok, most of the issues here have been addressed in the runtime or the test harness (or in pending PRs). There are still a couple of remaining minor items (like the PromiseRejectionEvent constructor) that will be addressed over time. For now, I'd like to get #119 landed then take a look at the regenerated matrix using an up to date workerd, then continue on from there.