idlharness.js is cumbersome for testing instances of Sensor

[foolip]

See discussion at https://github.com/w3c/web-platform-tests/pull/7763#pullrequestreview-69447993

The problem is that Sensor is abstract, only inheriting interfaces like AmbientLightSensor can be created. This means that in order to test instances, all of the idlharness.js tests need to have interface Sensor as part of the tested IDL. This leads to a lot of duplication (tests for hasReading would fail all over the place) and also requires splitting the IDL file for Generic Sensors, so that SensorErrorEvent isn't also tested everywhere.

@tobie, ideas? This should be similar to the Node hierarchy, but I don't know what the solution is there.

[tobie]

sensor.hasReading is just !!sensor.timestamp. I'm really not sure why it's at all necessary.

Is that the only attribute that's causing issues?

[foolip]

No, that's just a thing that would be annoying, the problem is that we'd have duplicate test coverage and need to split the IDL of this spec into two files.

[foolip]

How naughty would it be to to idl_array.members.Sensor.untested = false in all of the tests that add the generic sensors IDL as untested, to duplicate only Sensor coverage but not SensorErrorEvent?

[tobie]

Frankly, I sort of imagine the way forward as having one file per interface construct and letting idlharness require the ones it needs.

In the meantime, I tend to think that duplicating the SensorErrorEvent tests or splitting the file in two (if I understand the options well), aren't as bad as idl_array.members.Sensor.untested = false.

[foolip]

I would like to keep one IDL file per spec to make it easier for @mdittmer auto-update them, so given the choices I guess I'll just go for the duplicate coverage. Would idl_array.members.Sensor.untested = false be bad even given a leading assert_true('untested' in idl_array.members.Sensor) to guard against internal changes?

[tobie]

Maybe we should just expose an API (if one doesn't already exist) to do just that.

[foolip]

I could do that, there's something called prevent_multiple_testing which is in the same category. (I think, it claims to be documented earlier in the file but isn't AFAICT.)

The most scoped API shape might be idl_array.set_tested('Sensor'), would that be OK? That's assuming that it usually makes sense to opt in to duplicate testing for only a few things.

[tobie]

It's a sketchy space, I think prevent_multiple_testing does something completely different.

wrt to the API, I'm not sure what you suggest conveys well what we're trying to achieve (which tbh, isn't super clear either). Is this the case that we're only bumping into this issue for abstract classes? In that case, we probably want to see what's being done for other similar cases (as you noted above) and maybe aim for a more "descriptive" API (for lack of a better term). E.g.: idl_array.mark_abstract_interface('Sensor').

[foolip]

How about a second argument to add_idls that's an array of the members (interfaces etc.) to include, ignoring all others? For concrete sensors, it'd be idl_array.add_idls(generic_sensor, ['Sensor']).

[tobie]

LGTM. Would favor: idl_array.add_idls(generic_sensor, { except: ['Sensor'] }) or similar, though.

[foolip]

OK, I was actually going for an opt-in list, but we could have both. How about idl_array.add_idls(generic_sensor, { include: ['Sensor'] }) and exclude?

[tobie]

I went for except (or exclude), because we want the API to consistent with the default, imho. include feels weird in that regard. What about except/only or blocklist/allowlist?

[foolip]

@tobie, can you review the idlharness.js changes in https://github.com/w3c/web-platform-tests/pull/7861? I didn't read your reply before sending it, so let's repeat the naming discussion there :)