Skip to content
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.

Sign up for GitHub

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

Jump to bottom

test: Add TypedExecutionResult, check result type #659

Merged
merged 3 commits into from
Jan 5, 2021
Merged

test: Add TypedExecutionResult, check result type #659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
29e9c21
test: Fix the types of expected results
chfast Nov 22, 2020
1f7f6a4
test: Add TypedExecutionResult, check result type
chfast Nov 22, 2020
603aca8
test: Rework Result() matcher
chfast Nov 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions test/unittests/api_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,8 @@ TEST(api, find_exported_function)

auto opt_function = find_exported_function(*instance, "foo");
ASSERT_TRUE(opt_function);
EXPECT_THAT(opt_function->function(*instance, {}, 0), Result(42));
EXPECT_THAT(TypedExecutionResult(opt_function->function(*instance, {}, 0), ValType::i32),
Result(42_u32));
EXPECT_TRUE(opt_function->input_types.empty());
ASSERT_EQ(opt_function->output_types.size(), 1);
EXPECT_EQ(opt_function->output_types[0], ValType::i32);
Expand All @@ -296,7 +297,9 @@ TEST(api, find_exported_function)

auto opt_reexported_function = find_exported_function(*instance_reexported_function, "foo");
ASSERT_TRUE(opt_reexported_function);
EXPECT_THAT(opt_reexported_function->function(*instance, {}, 0), Result(42));
EXPECT_THAT(
TypedExecutionResult(opt_reexported_function->function(*instance, {}, 0), ValType::i32),
Result(42_u32));
EXPECT_TRUE(opt_reexported_function->input_types.empty());
ASSERT_EQ(opt_reexported_function->output_types.size(), 1);
EXPECT_EQ(opt_reexported_function->output_types[0], ValType::i32);
Expand Down
4 changes: 2 additions & 2 deletions test/unittests/execute_call_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ TEST(execute_call, call_indirect)

for (const auto param : {0u, 1u, 2u})
{
constexpr uint64_t expected_results[]{3, 2, 1};
constexpr uint32_t expected_results[]{3, 2, 1};

EXPECT_THAT(execute(module, 5, {param}), Result(expected_results[param]));
}
Expand Down Expand Up @@ -257,7 +257,7 @@ TEST(execute_call, call_indirect_imported_table)

for (const auto param : {0u, 1u, 2u})
{
constexpr uint64_t expected_results[]{3, 2, 1};
constexpr uint32_t expected_results[]{3, 2, 1};

EXPECT_THAT(execute(*instance, 0, {param}), Result(expected_results[param]));
}
Expand Down
14 changes: 7 additions & 7 deletions test/unittests/execute_control_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,7 +614,7 @@ TEST(execute_control, br_if_with_result)

for (const auto param : {0u, 1u})
{
constexpr uint64_t expected_results[]{
constexpr uint32_t expected_results[]{
3, // br_if not taken, result: 1 xor 2 == 3.
2, // br_if taken, result: 2, remaining item dropped.
};
Expand All @@ -639,7 +639,7 @@ TEST(execute_control, br_if_out_of_function)

for (const auto param : {0u, 1u})
{
constexpr uint64_t expected_results[]{
constexpr uint32_t expected_results[]{
1, // br_if not taken.
2, // br_if taken.
};
Expand Down Expand Up @@ -717,7 +717,7 @@ TEST(execute_control, br_table)

for (const auto param : {0u, 1u, 2u, 3u, 4u, 5u})
{
constexpr uint64_t expected_results[]{103, 102, 101, 100, 104, 104};
constexpr uint32_t expected_results[]{103, 102, 101, 100, 104, 104};

EXPECT_THAT(execute(parse(bin), 0, {param}), Result(expected_results[param]));
}
Expand Down Expand Up @@ -840,7 +840,7 @@ TEST(execute_control, if_smoke)

for (const auto param : {0u, 1u})
{
constexpr uint64_t expected_results[]{
constexpr uint32_t expected_results[]{
0, // no if branch.
4, // if branch.
};
Expand Down Expand Up @@ -871,7 +871,7 @@ TEST(execute_control, if_else_smoke)

for (const auto param : {0u, 1u})
{
constexpr uint64_t expected_results[]{
constexpr uint32_t expected_results[]{
2, // else branch.
1, // if branch.
};
Expand Down Expand Up @@ -908,7 +908,7 @@ TEST(execute_control, if_return_from_branch)

for (const auto param : {0u, 1u})
{
constexpr uint64_t expected_results[]{
constexpr uint32_t expected_results[]{
2, // else branch.
1, // if branch.
};
Expand Down Expand Up @@ -947,7 +947,7 @@ TEST(execute_control, if_br_from_branch)

for (const auto param : {0u, 1u})
{
constexpr uint64_t expected_results[]{
constexpr uint32_t expected_results[]{
2, // else branch.
21, // if branch.
};
Expand Down
4 changes: 2 additions & 2 deletions test/unittests/execute_numeric_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ using namespace fizzy::test;

namespace
{
ExecutionResult execute_unary_operation(Instr instr, TypedValue arg)
TypedExecutionResult execute_unary_operation(Instr instr, TypedValue arg)
{
const auto& instr_type = get_instruction_type_table()[static_cast<uint8_t>(instr)];
EXPECT_EQ(instr_type.inputs.size(), 1);
Expand All @@ -33,7 +33,7 @@ ExecutionResult execute_unary_operation(Instr instr, TypedValue arg)
return execute(*instantiate(std::move(module)), 0, {arg});
}

ExecutionResult execute_binary_operation(Instr instr, TypedValue lhs, TypedValue rhs)
TypedExecutionResult execute_binary_operation(Instr instr, TypedValue lhs, TypedValue rhs)
{
const auto& instr_type = get_instruction_type_table()[static_cast<uint8_t>(instr)];
EXPECT_EQ(instr_type.inputs.size(), 2);
Expand Down
7 changes: 4 additions & 3 deletions test/unittests/execute_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ TEST(execute, i32_load_all_variants)

const auto memory_fill = "deb0b1b2b3ed"_bytes;

constexpr std::tuple<Instr, uint64_t> test_cases[]{
constexpr std::tuple<Instr, uint32_t> test_cases[]{
{Instr::i32_load8_u, 0x000000b0},
{Instr::i32_load8_s, 0xffffffb0},
{Instr::i32_load16_u, 0x0000b1b0},
Expand Down Expand Up @@ -984,9 +984,10 @@ TEST(execute, reuse_args)

auto instance = instantiate(parse(wasm));

const std::vector<Value> args{20, 3};
const std::vector<Value> args{20_u64, 3_u64};
const auto expected = args[0].i64 % (args[0].i64 / args[1].i64);
EXPECT_THAT(execute(*instance, 0, args.data()), Result(expected));
EXPECT_THAT(
TypedExecutionResult(execute(*instance, 0, args.data()), ValType::i64), Result(expected));
EXPECT_THAT(args[0].i64, 20);
EXPECT_THAT(args[1].i64, 3);

Expand Down
116 changes: 113 additions & 3 deletions test/unittests/test_utils_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,11 @@ TEST(test_utils, from_hex)
EXPECT_THROW_MESSAGE(from_hex("FG"), std::out_of_range, "not a hex digit");
}

TEST(test_utils, result_signed_int)
TEST(test_utils, result_signed_int_typed)
{
EXPECT_THAT(ExecutionResult{Value{-1}}, Result(-1));
EXPECT_THAT(TypedExecutionResult(Value{-1}, ValType::i32), Result(-1));
constexpr auto v = std::numeric_limits<int32_t>::min();
EXPECT_THAT(ExecutionResult{Value{v}}, Result(v));
EXPECT_THAT(TypedExecutionResult(Value{v}, ValType::i32), Result(v));
}

TEST(test_utils, print_execution_result)
Expand Down Expand Up @@ -70,3 +70,113 @@ TEST(test_utils, print_c_execution_result)
str_value << FizzyExecutionResult{false, true, {42}};
EXPECT_EQ(str_value.str(), "result(42 [0x2a])");
}

TEST(test_utils, print_typed_execution_result)
{
std::stringstream str_trap;
str_trap << TypedExecutionResult{Trap, {}};
EXPECT_EQ(str_trap.str(), "trapped");

std::stringstream str_void;
str_void << TypedExecutionResult{Void, {}};
EXPECT_EQ(str_void.str(), "result()");

std::stringstream str_value_i32;
str_value_i32 << TypedExecutionResult{ExecutionResult{Value{42_u32}}, ValType::i32};
EXPECT_EQ(str_value_i32.str(), "result(42 [0x2a] (i32))");
str_value_i32.str({});
str_value_i32 << TypedExecutionResult{ExecutionResult{Value{0x80000000_u32}}, ValType::i32};
EXPECT_EQ(str_value_i32.str(), "result(2147483648 [0x80000000] (i32))");
str_value_i32.str({});
str_value_i32 << TypedExecutionResult{ExecutionResult{Value{-2_u32}}, ValType::i32};
EXPECT_EQ(str_value_i32.str(), "result(4294967294 [0xfffffffe] (i32))");


std::stringstream str_value_i64;
str_value_i64 << TypedExecutionResult{ExecutionResult{Value{42_u64}}, ValType::i64};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps have a second test with >2^32-1 value.

EXPECT_EQ(str_value_i64.str(), "result(42 [0x2a] (i64))");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test with negative integer values?

str_value_i64.str({});
str_value_i64 << TypedExecutionResult{ExecutionResult{Value{0x100000000_u64}}, ValType::i64};
EXPECT_EQ(str_value_i64.str(), "result(4294967296 [0x100000000] (i64))");
str_value_i64.str({});
str_value_i64 << TypedExecutionResult{ExecutionResult{Value{-3_u64}}, ValType::i64};
EXPECT_EQ(str_value_i64.str(), "result(18446744073709551613 [0xfffffffffffffffd] (i64))");

std::stringstream str_value_f32;
str_value_f32 << TypedExecutionResult{ExecutionResult{Value{1.125f}}, ValType::f32};
EXPECT_EQ(str_value_f32.str(), "result(1.125 (f32))");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test with negative floating point values?

str_value_f32.str({});
str_value_f32 << TypedExecutionResult{ExecutionResult{Value{-1.125f}}, ValType::f32};
EXPECT_EQ(str_value_f32.str(), "result(-1.125 (f32))");

std::stringstream str_value_f64;
str_value_f64 << TypedExecutionResult{ExecutionResult{Value{1.125}}, ValType::f64};
EXPECT_EQ(str_value_f64.str(), "result(1.125 (f64))");
str_value_f64.str({});
str_value_f64 << TypedExecutionResult{ExecutionResult{Value{-1.125}}, ValType::f64};
EXPECT_EQ(str_value_f64.str(), "result(-1.125 (f64))");
}

TEST(test_utils, result_value_matcher)
{
// Exercise every check in Result(value) implementation.
// The implementation and checks below are organized by the value's type in Result(value).
using testing::Not;

// TypedExecutionResult is required to be matched against Result(value).
EXPECT_THAT(ExecutionResult(Value{1_u64}), Not(Result(1_u64)));
gumb0 marked this conversation as resolved.
Show resolved Hide resolved

EXPECT_THAT(TypedExecutionResult(fizzy::Void, {}), Not(Result(0)));
EXPECT_THAT(TypedExecutionResult(fizzy::Trap, {}), Not(Result(0)));

EXPECT_THAT(TypedExecutionResult(Value{0.0f}, ValType::f32), Result(0.0f));
EXPECT_THAT(TypedExecutionResult(Value{0.0}, ValType::f64), Not(Result(0.0f)));

EXPECT_THAT(TypedExecutionResult(Value{0.0}, ValType::f64), Result(0.0));
EXPECT_THAT(TypedExecutionResult(Value{0.0f}, ValType::f32), Not(Result(0.0)));

EXPECT_THAT(TypedExecutionResult(Value{0_u64}, ValType::i64), Result(0_u64));
EXPECT_THAT(TypedExecutionResult(Value{0_u32}, ValType::i32), Not(Result(0_u64)));

EXPECT_THAT(TypedExecutionResult(Value{0_u32}, ValType::i32), Result(0_u32));

// For non-negative values zero-extension is conveniently allowed.
EXPECT_THAT(TypedExecutionResult(Value{0_u64}, ValType::i64), Result(0));
EXPECT_THAT(TypedExecutionResult(Value{0_u64}, ValType::i64), Result(0_u32));

EXPECT_THAT(TypedExecutionResult(Value{-1_u32}, ValType::i32), Result(-1));
EXPECT_THAT(TypedExecutionResult(Value{-1_u32}, ValType::i32), Result(-1_u32));
EXPECT_THAT(TypedExecutionResult(Value{-1_u32}, ValType::i32), Not(Result(-1_u64)));

EXPECT_THAT(TypedExecutionResult(Value{-1_u64}, ValType::i64), Result(-1_u64));
EXPECT_THAT(TypedExecutionResult(Value{-1_u64}, ValType::i64), Not(Result(-1)));
gumb0 marked this conversation as resolved.
Show resolved Hide resolved
EXPECT_THAT(TypedExecutionResult(Value{-1_u64}, ValType::i64), Not(Result(-1_u32)));

// Comparing with non-wasm types always return false.
EXPECT_THAT(TypedExecutionResult(Value{1_u32}, ValType::i32), Not(Result(uint8_t{1})));
EXPECT_THAT(TypedExecutionResult(Value{1_u64}, ValType::i64), Not(Result(uint8_t{1})));
}

TEST(test_utils, result_value_matcher_explain_missing_result_type)
{
const auto result = testing::internal::MakePredicateFormatterFromMatcher(Result(1_u64))(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well this is clearly something internal to google.mock, I'm not sure it's a good idea to use it. For coverage tests with Not above should be sufficient.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah for coverage you mentioned that tests wth Not don't print output... Ok fine, we can leave this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to test the message output. Using Not() will get us coverage (if IsInterested() is not used) but we cannot test the message. This was the previous version. I can roll back to that one if preferred.

"<value>", ExecutionResult(Value{1_u64}));
EXPECT_FALSE(result);
EXPECT_STREQ(result.message(),
"Value of: <value>\n"
"Expected: result 1\n"
" Actual: result(1 [0x1]) (of type fizzy::ExecutionResult), "
"TypedExecutionResult expected");
}

TEST(test_utils, result_value_matcher_explain_non_wasm_type)
{
const auto result = testing::internal::MakePredicateFormatterFromMatcher(Result(char{1}))(
"<value>", TypedExecutionResult(Value{1_u32}, ValType::i32));
EXPECT_FALSE(result);
EXPECT_STREQ(result.message(),
"Value of: <value>\n"
"Expected: result '\\x1' (1)\n"
" Actual: result(1 [0x1] (i32)) (of type fizzy::test::TypedExecutionResult), "
"expected value has non-wasm type");
}
38 changes: 37 additions & 1 deletion test/utils/asserts.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ inline void output_result(std::ostream& os, ResultT result)
const auto format_flags = os.flags();
os << "result(";
if (result.has_value)
os << result.value.i64 << " [0x" << std::hex << result.value.i64 << "]";
os << std::dec << result.value.i64 << " [0x" << std::hex << result.value.i64 << "]";
os << ")";
os.flags(format_flags);
}
Expand All @@ -38,4 +38,40 @@ std::ostream& operator<<(std::ostream& os, ExecutionResult result)
output_result(os, result);
return os;
}

namespace test
{
std::ostream& operator<<(std::ostream& os, const TypedExecutionResult& result)
{
const auto format_flags = os.flags();

if (result.has_value)
{
os << "result(";
switch (result.type)
{
case ValType::i32:
os << std::dec << result.value.as<uint32_t>() << " [0x" << std::hex
<< result.value.as<uint32_t>() << "] (i32)";
break;
case ValType::i64:
os << std::dec << result.value.i64 << " [0x" << std::hex << result.value.i64
<< "] (i64)";
break;
case ValType::f32:
os << result.value.f32 << " (f32)";
break;
case ValType::f64:
os << result.value.f64 << " (f64)";
break;
}
os << ")";
}
else
os << ExecutionResult{result};

os.flags(format_flags);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah this is the reset. So is decimal mode on by default according to the std?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. It resets to what state it was before. So explicit dec is needed to the first part.

return os;
}
} // namespace test
} // namespace fizzy
57 changes: 52 additions & 5 deletions test/utils/asserts.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@
#include <fizzy/fizzy.h>
#include <gmock/gmock.h>
#include <test/utils/floating_point_utils.hpp>
#include <test/utils/typed_value.hpp>
#include <iosfwd>
#include <type_traits>

MATCHER(Traps, "") // NOLINT(readability-redundant-string-init)
{
Expand All @@ -23,13 +25,56 @@ MATCHER(Result, "empty result")

MATCHER_P(Result, value, "") // NOLINT(readability-redundant-string-init)
{
if (arg.trapped || !arg.has_value)
using namespace fizzy;

// Require the arg to be TypedExecutionResult.
// This can be a static_assert, but just returning false and failing a test provides better
// location of the error.
using result_type = std::remove_cv_t<std::remove_reference_t<arg_type>>;
if constexpr (!std::is_same_v<result_type, test::TypedExecutionResult>)
{
if (result_listener->IsInterested())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this part of google.test documented?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this in implementation of builtin matchers. This seems not to be strictly needed, but this is false e.q. when Not() is used.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be undocumented feature. The only mention of result_listener I found uses only operator<< https://github.com/google/googletest/blob/master/googlemock/docs/cheat_sheet.md#defining-matchers

*result_listener << "TypedExecutionResult expected";
return false;
}
gumb0 marked this conversation as resolved.
Show resolved Hide resolved
else
{
if (arg.trapped || !arg.has_value)
return false;

if constexpr (std::is_floating_point_v<value_type>)
return arg.value.template as<value_type>() == fizzy::test::FP{value};
else // always check 64 bit of result for all integers, including 32-bit results
return arg.value.i64 == static_cast<std::make_unsigned_t<value_type>>(value);
if constexpr (std::is_same_v<value_type, float>)
{
return arg.type == ValType::f32 && arg.value.f32 == test::FP{value};
}

if constexpr (std::is_same_v<value_type, double>)
{
return arg.type == ValType::f64 && arg.value.f64 == test::FP{value};
}

if constexpr (std::is_integral_v<value_type> && sizeof(value_type) == sizeof(uint64_t))
{
return arg.type == ValType::i64 && arg.value.i64 == static_cast<uint64_t>(value);
}

if constexpr (std::is_integral_v<value_type> && sizeof(value_type) == sizeof(uint32_t))
{
if (arg.type == ValType::i32)
{
return arg.value.i64 == static_cast<uint32_t>(value);
}
else if (arg.type == ValType::i64 && value >= 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you plan to remove this and change the tests at some point?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. Probably make sense to always use Result(0_u32) or Result(0_u64) only.

{
// Here allow convenient zero-extension of the expected value u32 -> u64.
return arg.value.i64 == static_cast<uint32_t>(value);
}
return false;
}

if (result_listener->IsInterested())
*result_listener << "expected value has non-wasm type";
return false;
}
}

MATCHER(CTraps, "") // NOLINT(readability-redundant-string-init)
Expand Down Expand Up @@ -101,4 +146,6 @@ inline uint32_t as_uint32(fizzy::Value value)
EXPECT_EQ(value.i64 & 0xffffffff00000000, 0);
return static_cast<uint32_t>(value.i64);
}

std::ostream& operator<<(std::ostream& os, const TypedExecutionResult&);
} // namespace fizzy::test
Loading