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.

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

Type hardening in tests #687

Merged
merged 3 commits into from
Jan 11, 2021
Merged

Type hardening in tests #687

merged 3 commits into from
Jan 11, 2021

Conversation

chfast
Copy link
Collaborator

@chfast chfast commented Jan 8, 2021

No description provided.

@chfast chfast requested review from gumb0 and axic January 8, 2021 12:28
@@ -44,10 +44,10 @@ TEST(api, execution_result_void)

TEST(api, execution_result_value)
{
const ExecutionResult result = Value{1234};
const ExecutionResult result = Value{1234_u32};
Copy link
Member

Choose a reason for hiding this comment

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

If this i32 now, shouldn't there be another test for i64/f32/f64 (pick one or more?)?

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 what this test is about. Seems to testing implicit Value -> ExecutionResult conversion.

Copy link
Member

Choose a reason for hiding this comment

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

@gumb0 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah this tests ExecutionResult constructor taking Value

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah this tests ExecutionResult constructor taking Value

Right, I guess implicit one, because there's also a test for explicit constuction below.

@@ -510,8 +510,7 @@ TEST(execute_numeric, i64_extend_i32_u_2)
"0061736d0100000001060160017f017e030201000a1201100042effdb6f5fdddefd65e1a2000ad0b");

auto instance = instantiate(parse(wasm));
const auto r = execute(*instance, 0, {0xff000000});
EXPECT_THAT(r, Result(uint64_t{0x00000000ff000000}));
EXPECT_THAT(execute(*instance, 0, {0xff000000}), Result(0x00000000ff000000_u64));
Copy link
Member

Choose a reason for hiding this comment

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

Is this the last such instance or why pick on this one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The last case, at least in files using _u64.

@codecov
Copy link

codecov bot commented Jan 8, 2021

Codecov Report

Merging #687 (1042781) into master (c7ed699) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #687   +/-   ##
=======================================
  Coverage   99.31%   99.31%           
=======================================
  Files          72       72           
  Lines       10148    10148           
=======================================
  Hits        10078    10078           
  Misses         70       70           
Flag Coverage Δ
spectests 91.50% <ø> (ø)
unittests 99.31% <100.00%> (ø)

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/capi_test.cpp 100.00% <100.00%> (ø)
test/unittests/execute_numeric_test.cpp 100.00% <100.00%> (ø)
test/utils/asserts.hpp 97.29% <100.00%> (+0.07%) ⬆️

else
{
if (result_listener->IsInterested())
*result_listener << "unsupported type";
Copy link
Member

Choose a reason for hiding this comment

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

Do we want a test for this?

@chfast chfast merged commit 26bb712 into master Jan 11, 2021
@chfast chfast deleted the test_type_hardening branch January 11, 2021 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants