-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(expect): add toBeInstanceOf matcher #2389
feat(expect): add toBeInstanceOf matcher #2389
Conversation
active_test_expectation_counter.actual += 1; | ||
|
||
const expected_value = arguments[0]; | ||
if (!expected_value.jsType().isFunction()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you either add a TODO here to check that the function is a constructor or add a binding for isConstructor
?
Checking that the JSType is one of the function types isn't precise enough. Functions can callable and/or constructable. The JSType can be whatever we want, though in most cases it will be one of the 3 that JSType currently checks (just not every case, in theory)
if (!expected_value.jsType().isFunction()) { | |
if (!expected_value.isConstructable()) { |
This isConstructor
function doesn't exist, but it would be nearly identical to isCallable
:
bun/src/bun.js/bindings/bindings.zig
Lines 3528 to 3530 in 4c38798
pub fn isCallable(this: JSValue, vm: *VM) bool { | |
return cppFn("isCallable", .{ this, vm }); | |
} |
bun/src/bun.js/bindings/bindings.cpp
Lines 2425 to 2428 in 4c38798
bool JSC__JSValue__isCallable(JSC__JSValue JSValue0, JSC__VM* arg1) | |
{ | |
return JSC::JSValue::decode(JSValue0).isCallable(); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this! One comment
There will be some test failures but don't worry about it - not your fault. |
OK,I will check it tmr. |
I'll just merge, it's an improvement over status quo and we can clean it up after |
implement
toBeInstanceOf
related issue: #1825