fix(accessibility): reject unknown audit types before they reach the device - #306
fix(accessibility): reject unknown audit types before they reach the device#306navin772 wants to merge 1 commit into
Conversation
| this.auditTypeNames ??= new Set(await this.getSupportedAuditTypes(options)); | ||
| const unknown = auditTypes.filter((auditType) => !this.auditTypeNames?.has(auditType)); | ||
| if (unknown.length > 0) { | ||
| throw new Error( |
There was a problem hiding this comment.
can it ever happen that this.auditTypeNames is empty?
| const unknown = auditTypes.filter((auditType) => !this.auditTypeNames?.has(auditType)); | ||
| if (unknown.length > 0) { | ||
| throw new Error( | ||
| `Unknown audit type(s) ${unknown.map((auditType) => JSON.stringify(auditType)).join(', ')}; ` + |
There was a problem hiding this comment.
consider using util.pluralize
| : undefined; | ||
|
|
||
| try { | ||
| await this.assertKnownAuditTypes(auditTypes, options); |
There was a problem hiding this comment.
dont forward options here...
the caller's audit-completion timeoutMs becomes the reply timeout for the deviceAllSupportedAuditTypes invoke (overriding the 15s transport default), so a long audit budget can block minutes in pre-flight and total wait approaches 2× timeoutMs.
|
|
||
| it('rejects an unknown setting identifier instead of silently doing nothing', async function () { | ||
| it('rejects an unknown setting identifier instead of silently doing nothing', async function (t) { | ||
| if (skipWithoutSettingWrites(t)) { |
There was a problem hiding this comment.
These three tests reject client side before the write selector is ever invoked, so they shouldnt be skipped behind skipWithoutSettingWrites
|
|
||
| /** Reports why a test is being skipped, or null when it may run. */ | ||
| async function skipReason(): Promise<string | null> { | ||
| if (!supportsSettingWrites) { |
There was a problem hiding this comment.
this gates on deviceUpdateAccessibilitySetting
but 2-3 tests only use deviceUpdateAccessibilitySetting gates on reset selector.
may be add the write only for clear session override selector
| ]; | ||
|
|
||
| for (const [label, auditTypes] of POISON) { | ||
| it(`rejects ${label} without contacting the device`, async function () { |
There was a problem hiding this comment.
on a cold cacge the first rejection doesnt contact the device here
| await assert.rejects(() => service!.runAudit([types[0], 'bogusType'], {timeoutMs: 20000}), /Unknown audit type/); | ||
| }); | ||
|
|
||
| it('still accepts an empty list, which the device reads as every type', async function () { |
There was a problem hiding this comment.
this duplicates above test in file "accepts an empty audit-type list without hanging"
| }); | ||
|
|
||
| it('leaves the connection usable after a rejected audit', async function (t) { | ||
| const types = await service!.getSupportedAuditTypes(); |
There was a problem hiding this comment.
The recovery audit only needs to prove the connection still works. the purpose of test is only checks that the connection still works after a rejection, checkignf for all is a time waste
may be
it('leaves the connection usable after a rejected audit', async function () {
await assert.rejects(
service!.runAudit(['nopeNotAType'], {timeoutMs: 20000}),
/Unknown audit type/,
);
const issues = await service!.runAudit(['testTypeContrast'], {timeoutMs: 60000});
assert.ok(Array.isArray(issues));
});
3 participants
An audit type the daemon doesn't implement makes it return neither issues nor a completion, and that connection can never run another audit, the service looks alive (
getApiVersionstill works) while everyrunAudittimes out.runAuditnow validates againstgetSupportedAuditTypes()and throws locally, caching the list per connection. Matching is exact: wrong case, surrounding whitespace and a baretestTypeprefix all wedge the daemon too. An empty list still passes through, the device reads it as "every type".