Add test for integer limits #20353
Add test for integer limits #20353
Conversation
While debugging an issue with integers in #20344, I found some cases where conversions currently fail or don't behave as expected in WASM64 mode. In the end, I decided to add a separate test that solely tests all integer limits and ensures they roundtrip between JS and C++ as expected. I don't have time to look into fixing uncovered issues, but I thought these (currently failing) tests might be a helpful start on their own.
|
@brendandahl @sbc100 Any chance one of you could look into this one? I'm not familiar with wasm64 yet and not sure what expectations for those types are, yet running into those issues regularly even on subtle changes to Embind. |
| #define TEST_NUM_LIMITS(T) ok &= test_num_limits<T>(#T) | ||
|
|
||
| int main() { | ||
| bool ok = true; |
There was a problem hiding this comment.
Unrelated to the actual failure we (I) tend to prefer just using assert(..) everywhere rather than accumulating the failures in some kind of success or ok variable.
It means test will exit on first failure but I've found that that is almost always what I want anyway (since it avoids having to page back through reams of output to find the first failure when debugging).
This also makes the tests simpler since you can basically remove ok from here and everywhere else in this file.
There was a problem hiding this comment.
It means test will exit on first failure but I've found that that is almost always what I want anyway
That's what I almost always want too, but I believe in this case an exception was warranted: there are various failure edge cases and the goal was to uncover all of them in a flat list. Otherwise we wouldn't know what other bugs are there until we fix the first one and the next one is uncovered and so on.
When all those bugs are fixed, we can revert this to simpler version if that's desirable, but for now it was more of a "show all types that are broken" exploration.
There was a problem hiding this comment.
(worth noting that it still doesn't fully realise that goal because at some point test crashes with JS exception after regular value mismatches, so there are probably more things broken after that line)
|
Had a brief look at the exception again and looks like at least the crash comes from wrappers in |
While debugging an issue with integers in #20344, I found some cases where conversions currently fail or don't behave as expected in WASM64 mode.
In the end, I decided to add a separate test that solely tests all integer limits and ensures they roundtrip between JS and C++ as expected.
I don't have time to look into fixing uncovered issues, but I thought these (currently failing) tests might be a helpful start on their own.