-
Notifications
You must be signed in to change notification settings - Fork 34
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
Support floats in spectest runner #445
Conversation
@@ -9,7 +9,7 @@ add_test( | |||
set_tests_properties( | |||
fizzy/smoketests/spectests/default | |||
PROPERTIES | |||
PASS_REGULAR_EXPRESSION "PASSED 23, FAILED 0, SKIPPED 7" | |||
PASS_REGULAR_EXPRESSION "PASSED 26, FAILED 1, SKIPPED 3" |
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.
1 failure is for float global, will be fixed in #446
test/spectests/spectests.cpp
Outdated
template <> | ||
fizzy::Value json_to_value<float>(const json& v) | ||
{ | ||
return std::stoull(v.get<std::string>()); |
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.
Why using "to unsigned integer" for floating point values?
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.
wast2json
puts them as 64-bit integers to json
expected_failed: 9 | ||
expected_skipped: 6336 | ||
expected_passed: 5577 | ||
expected_failed: 13 |
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.
4 new failures are all in globals.wast
, will be fixed in #446
Codecov Report
@@ Coverage Diff @@
## master #445 +/- ##
==========================================
- Coverage 99.51% 99.51% -0.01%
==========================================
Files 52 52
Lines 14791 14785 -6
==========================================
- Hits 14719 14713 -6
Misses 72 72 |
const auto arg_type = v.at("type").get<std::string>(); | ||
if (arg_type != "i32" && arg_type != "i64" && arg_type != "f32" && arg_type != "f64") | ||
{ | ||
skip("Unsupported value type '" + arg_type + "'."); |
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.
This is not covered by unit tests.
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.
Added tests.
} | ||
|
||
// Values of all types are serialized to JSON as integers. | ||
// Value type will handle correct conversions. |
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.
How? Why not keep the old json_to_value<int32_t>(arg.at("value"))
way?
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.
It's just simpler without additional switch
and template.
Presumably wast2json
does the opposite conversion - typed value into unsigned integer. We could reinterpret_cast it back to correct type, but Value(correct_type_value)
constructor does the same as Value(reinterpret_cast<unsigned long long>(correct_type_value))
, which is the idea of Value
type...
Depends on #424