[js-api] Add test case for externref global#1983
Conversation
| ]) { | ||
| global.value = value; |
There was a problem hiding this comment.
Isn't this going to throw an assertion in step 5 because you didn't pass mutable: true in the constructor?
| () => null | ||
| ]) { | ||
| global.value = value; | ||
| assert_Global(global, value); | ||
| } | ||
| }, "externref global"); |
There was a problem hiding this comment.
This case isn't testing the constructor, which is what this file is for. I believe there's a file for the value getter/setter nearby
There was a problem hiding this comment.
Good catch. After checking how I missed that, I realized that I missed one "include" in my invocation for this file and that script was reponsible for actually executing the test cases. (It isn't very obvious to me how to run these scripts in e.g. d8.)
I addressed this issue now by splitting it up into testing the constructor and a separate test case for get-set.
| () => null | ||
| ]) { | ||
| global.value = value; | ||
| assert_Global(global, value); | ||
| } | ||
| }, "externref global"); |
There was a problem hiding this comment.
Good catch. After checking how I missed that, I realized that I missed one "include" in my invocation for this file and that script was reponsible for actually executing the test cases. (It isn't very obvious to me how to run these scripts in e.g. d8.)
I addressed this issue now by splitting it up into testing the constructor and a separate test case for get-set.
| () => null | ||
| ]) { | ||
| global.value = value; | ||
| assert_equals(global.value, value); |
There was a problem hiding this comment.
See also #1984 (comment), this tests -0 as one of its inputs. assert_equals already treats 0 and -0 as different values:
assert_equals: expected -0 but got 0
Let me know if this is fine given that the current spec text isn't very precise.
|
Can this be merged? |
2 participants
We should add some basic coverage for externref globals.
(We should extend this if #1980 results in adding more allowed types.)