test: Add TypedExecutionResult, check result type #659

chfast · 2020-11-23T10:05:44Z

Introduce TypedExecutionResult which takes the result type from the function type.
Adjust Result() matcher to expect TypedExecutionResult and perform type-safe value checks.
The Result() is more liberal comparing with TypedValue: it allows u64 - u32 and u64 - i32 if not negative compares.
Add Result() unit tests.

codecov · 2020-11-23T10:12:11Z

Codecov Report

Merging #659 (603aca8) into master (12c0423) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #659      +/-   ##
==========================================
+ Coverage   99.28%   99.30%   +0.01%     
==========================================
  Files          72       72              
  Lines        9954    10067     +113     
==========================================
+ Hits         9883     9997     +114     
+ Misses         71       70       -1

Flag	Coverage Δ
spectests	`91.50% <ø> (ø)`
unittests	`99.30% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
test/unittests/api_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/execute_call_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/execute_control_test.cpp	`99.46% <100.00%> (ø)`
test/unittests/execute_numeric_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/execute_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/test_utils_test.cpp	`100.00% <100.00%> (ø)`
test/utils/asserts.cpp	`100.00% <100.00%> (ø)`
test/utils/asserts.hpp	`97.22% <100.00%> (+5.55%)`	⬆️
test/utils/execute_helpers.hpp	`100.00% <100.00%> (ø)`
test/utils/typed_value.hpp	`100.00% <100.00%> (ø)`

test/utils/asserts.cpp

gumb0 · 2020-11-30T14:59:43Z

Please rebase.

chfast · 2020-12-01T09:18:11Z

Please rebase.

I'm not touching this PR until #655 is merged.

gumb0 · 2020-12-01T16:39:48Z

Please rebase.

I'm not touching this PR until #655 is merged.

Then I'm not reviewing it till then.

axic · 2020-12-29T21:32:42Z

Needs rebase, and possibly merging #655 first.

chfast · 2020-12-30T11:34:26Z

Rebased, without checking the implementation.

axic · 2020-12-31T16:24:15Z

test/utils/asserts.hpp

        return false;
+    }

    if (arg.trapped || !arg.has_value)
        return false;


Perhaps display a reason here too if you do that above?

This is not needed because "trapped" execution is printed as "trapped", etc.

Random example:

Value of: execute(*instance, 0, {65534}) Expected: result 1 Actual: trapped (of type fizzy::test::TypedExecutionResult)

axic · 2020-12-31T19:20:53Z

test/utils/asserts.cpp

+        switch (result.type)
+        {
+        case ValType::i32:
+            os << result.value.as<uint32_t>() << " [0x" << std::hex << result.value.as<uint32_t>()


Switch back to dec?

And/or switch to dec explicitly for the first part?

axic · 2020-12-31T19:22:33Z

test/utils/asserts.cpp

+    else
+        os << ExecutionResult{result};
+
+    os.flags(format_flags);


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

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

axic · 2020-12-31T19:23:04Z

test/unittests/test_utils_test.cpp

+    EXPECT_EQ(str_value_i32.str(), "result(42 [0x2a] (i32))");
+
+    std::stringstream str_value_i64;
+    str_value_i64 << TypedExecutionResult{ExecutionResult{Value{42_u64}}, ValType::i64};


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

axic · 2020-12-31T19:23:37Z

test/unittests/test_utils_test.cpp

+    EXPECT_EQ(str_void.str(), "result()");
+
+    std::stringstream str_value_i32;
+    str_value_i32 << TypedExecutionResult{ExecutionResult{Value{42}}, ValType::i32};


Does this constructor verify that the input is <=2^32-? If not, can you put a test with a large value?

Here you mean the Value{} constructor. It currently has overloads for many integer types and maps them to wasm types. No additional check is required.

axic · 2020-12-31T19:23:48Z

test/unittests/test_utils_test.cpp

+
+    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))");


Can you add a test with negative floating point values?

axic · 2020-12-31T19:23:57Z

test/unittests/test_utils_test.cpp

+
+    std::stringstream str_value_i64;
+    str_value_i64 << TypedExecutionResult{ExecutionResult{Value{42_u64}}, ValType::i64};
+    EXPECT_EQ(str_value_i64.str(), "result(42 [0x2a] (i64))");


Can you add a test with negative integer values?

axic · 2020-12-31T19:42:15Z

test/utils/execute_helpers.hpp

@@ -31,13 +31,15 @@ inline ExecutionResult execute(
    std::transform(std::cbegin(typed_args), std::cend(typed_args), std::begin(args),
        [](const auto& typed_arg) { return typed_arg.value; });

-    return fizzy::execute(instance, func_idx, args.data());
+    const auto result = fizzy::execute(instance, func_idx, args.data(), depth);
+    const auto result_type = func_type.outputs.empty() ? ValType{} : func_type.outputs[0];


Perhaps an assertion that outputs.size() <= 1?

test/unittests/test_utils_test.cpp

axic · 2020-12-31T19:43:08Z

test/utils/asserts.hpp

    // 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>)
+    {
+        *result_listener << "TypedExecutionResult expected";


Can the test for texts include this?

axic · 2020-12-31T19:43:32Z

test/utils/asserts.hpp

 MATCHER_P(Result, value, "")  // NOLINT(readability-redundant-string-init)
 {
    using namespace fizzy;

-    static_assert(is_any_of<value_type, uint32_t, int32_t, uint64_t, int64_t, float, double>);


Why remove this?

Not strictly needed now and the Result() will just return false.

test/unittests/test_utils_test.cpp

gumb0 · 2021-01-05T11:13:30Z

test/utils/asserts.hpp

+    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())


Where is this part of google.test documented?

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.

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

test/utils/asserts.hpp

gumb0 · 2021-01-05T11:22:42Z

test/utils/asserts.hpp

+            {
+                return arg.value.i64 == static_cast<uint32_t>(value);
+            }
+            else if (arg.type == ValType::i64 && value >= 0)


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

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

test/unittests/test_utils_test.cpp

gumb0 · 2021-01-05T12:11:37Z

test/unittests/test_utils_test.cpp

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


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.

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

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.

gumb0

Looks good, one minor test suggestion #659 (comment)

chfast force-pushed the test_typed_result branch 3 times, most recently from 60eecdf to e586811 Compare November 23, 2020 12:28

chfast requested review from gumb0 and axic November 23, 2020 12:34

chfast force-pushed the test_typed_result branch from e586811 to 78b39a0 Compare November 23, 2020 13:30

gumb0 reviewed Nov 23, 2020

View reviewed changes

test/utils/asserts.cpp Outdated Show resolved Hide resolved

chfast force-pushed the test_typed_value branch 3 times, most recently from acdec1b to 678af2f Compare November 23, 2020 19:15

chfast mentioned this pull request Nov 25, 2020

test: Check the types of execution arguments #655

Merged

chfast force-pushed the test_typed_value branch 3 times, most recently from 4452038 to e0b7bd2 Compare November 27, 2020 14:47

chfast force-pushed the test_typed_value branch 4 times, most recently from b96047f to 9c42204 Compare December 29, 2020 19:43

axic mentioned this pull request Dec 29, 2020

Add uint32_t i32 field to Value union #517

Merged

Base automatically changed from test_typed_value to master December 30, 2020 09:04

chfast force-pushed the test_typed_result branch from 78b39a0 to a12cd10 Compare December 30, 2020 10:58

chfast force-pushed the test_typed_result branch from ac097b5 to f6084a2 Compare December 30, 2020 16:48

axic reviewed Dec 31, 2020

View reviewed changes

test/unittests/test_utils_test.cpp Outdated Show resolved Hide resolved

axic reviewed Dec 31, 2020

View reviewed changes

test/unittests/test_utils_test.cpp Show resolved Hide resolved

chfast force-pushed the test_typed_result branch 2 times, most recently from d975992 to 57fcaf4 Compare January 4, 2021 08:53

axic approved these changes Jan 4, 2021

View reviewed changes

gumb0 reviewed Jan 5, 2021

View reviewed changes

test/utils/asserts.hpp Show resolved Hide resolved

gumb0 reviewed Jan 5, 2021

View reviewed changes

chfast added 2 commits January 5, 2021 12:48

test: Fix the types of expected results

29e9c21

test: Add TypedExecutionResult, check result type

1f7f6a4

chfast force-pushed the test_typed_result branch from 57fcaf4 to 97ff588 Compare January 5, 2021 12:01

gumb0 reviewed Jan 5, 2021

View reviewed changes

test/unittests/test_utils_test.cpp Show resolved Hide resolved

gumb0 reviewed Jan 5, 2021

View reviewed changes

gumb0 approved these changes Jan 5, 2021

View reviewed changes

test: Rework Result() matcher

603aca8

chfast force-pushed the test_typed_result branch from 97ff588 to 603aca8 Compare January 5, 2021 12:36

chfast merged commit 9a25602 into master Jan 5, 2021

chfast deleted the test_typed_result branch January 5, 2021 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Add TypedExecutionResult, check result type #659

test: Add TypedExecutionResult, check result type #659

chfast commented Nov 23, 2020 •

edited

Loading

codecov bot commented Nov 23, 2020 •

edited

Loading

gumb0 commented Nov 30, 2020

chfast commented Dec 1, 2020

gumb0 commented Dec 1, 2020

axic commented Dec 29, 2020

chfast commented Dec 30, 2020

axic Dec 31, 2020

chfast Dec 31, 2020

axic Dec 31, 2020

axic Dec 31, 2020

axic Dec 31, 2020

chfast Jan 4, 2021

axic Dec 31, 2020

axic Dec 31, 2020

chfast Jan 4, 2021

axic Dec 31, 2020

axic Dec 31, 2020

axic Dec 31, 2020

axic Dec 31, 2020

axic Dec 31, 2020

chfast Jan 4, 2021

gumb0 Jan 5, 2021

chfast Jan 5, 2021

gumb0 Jan 5, 2021

gumb0 Jan 5, 2021

chfast Jan 5, 2021

gumb0 Jan 5, 2021

gumb0 Jan 5, 2021

chfast Jan 5, 2021

gumb0 left a comment

test: Add TypedExecutionResult, check result type #659

test: Add TypedExecutionResult, check result type #659

Conversation

chfast commented Nov 23, 2020 • edited Loading

codecov bot commented Nov 23, 2020 • edited Loading

Codecov Report

gumb0 commented Nov 30, 2020

chfast commented Dec 1, 2020

gumb0 commented Dec 1, 2020

axic commented Dec 29, 2020

chfast commented Dec 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 left a comment

Choose a reason for hiding this comment

chfast commented Nov 23, 2020 •

edited

Loading

codecov bot commented Nov 23, 2020 •

edited

Loading