Assertion flaw when testing functions we expect to throw an error
Assertion flaw when testing functions we expect to throw an error
For example in modules/matrix-bridge/test/gitter-bridge-test.js#L990-1000
, we use an assertion that looks like the following:
try {
await gitterBridge.onDataChange({ /* ... */ });
assert.fail('expected Matrix room join to fail because of our stub');
} catch (err) {
assert(err);
}
The code will pass regardless of if the onDataChange
function throws or not:
- If
onDataChange
throws an error, it will be asserted by ourassert(err);
as expected - But if
onDataChange
passes, ourassert.fail
will throw an error andassert(err);
will still happily accept that error❌
Fixes
We could check the error instance/type. We already do this in a few places in the codebase like modules/gitlab/test/gitlab-issuable-service-test.js#L62-64
if (err instanceof assert.AssertionError) {
throw err;
}
Affected tests
-
modules/cache-wrapper/test/cache-wrapper-test.js#L36-42
-
modules/matrix-bridge/test/gitter-bridge-test.js
-
modules/matrix-bridge/test/matrix-utils-test.js
-
modules\matrix-bridge\test\matrix-event-handler-test.js
-
Related but mis-use of assert.throws
modules/gitlab/test/gitlab-user-service-test.js#L50-52
These also have the same problem but do check err.status
which will only be relevant to the correct error
-
modules\permissions\test\security-descriptor-write-validator-test.js
-
modules/rooms/test/room-with-policy-service-test.js
-
modules/users/test/virtual-user-service-test.js
-
test/integration/services/group-with-policy-service-test.js
-
We do a bunch of assert.ok(false, ...
stuff but we check theerr.status
Edited by Eric Eastwood