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

test: Refactor utils for binop/unop tests #651

Merged
merged 4 commits into from
Jan 5, 2021
Merged

Conversation

chfast
Copy link
Collaborator

@chfast chfast commented Nov 18, 2020

No description provided.

@axic
Copy link
Member

axic commented Dec 29, 2020

I think some of these changes are in conflict with (or duplicated in) #655, lets wait for that.

@axic axic marked this pull request as draft December 29, 2020 22:07
@chfast chfast changed the base branch from master to test_typed_result December 30, 2020 18:54
@chfast chfast marked this pull request as ready for review December 30, 2020 18:54
@chfast chfast requested review from gumb0 and axic December 30, 2020 18:54
@codecov
Copy link

codecov bot commented Dec 30, 2020

Codecov Report

Merging #651 (ff747ef) into master (9a25602) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #651   +/-   ##
=======================================
  Coverage   99.30%   99.30%           
=======================================
  Files          72       72           
  Lines       10067    10090   +23     
=======================================
+ Hits         9997    10020   +23     
  Misses         70       70           
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/execute_numeric_test.cpp 100.00% <100.00%> (ø)


TypedExecutionResult execute_unary_operation(Instr instr, TypedValue arg)
{
return create_unary_operation_executor(instr)(arg);
Copy link
Member

Choose a reason for hiding this comment

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

Will these create helpers used later in other PRs?

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 i64_eqz is using it.

Copy link
Member

Choose a reason for hiding this comment

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

The point is to avoid type check or why is it needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is not needed, but it is lame to create module+instance for every test case. Here we create a lambda capturing the instance and we can execute multiple test cases.

Copy link
Member

Choose a reason for hiding this comment

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

Well, the fresh instance gives the benefit, that nothing is dirty anywhere in the instance. However this way perhaps we could catch other bugs/problems caused by some uncleaned state.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is not needed, but it is lame to create module+instance for every test case. Here we create a lambda capturing the instance and we can execute multiple test cases.

But there are many tests still using execute_unary_operation / execute_binary_operation, why leave it inconsistent?

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 PR mostly enables you to do the change. I needed it for more eqz test cases. For other instructions I prefer to go one-by-one later and add more test cases (something more than 42). But I can syntactically replace the code right now if you prefer all at once (will not add more cases though).

Copy link
Collaborator

@gumb0 gumb0 Jan 4, 2021

Choose a reason for hiding this comment

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

I would prefer to replace all of them here, if it's easy. (and remove execute_* functions)

@chfast chfast force-pushed the test_typed_result branch 2 times, most recently from d975992 to 57fcaf4 Compare January 4, 2021 08:53
{
const auto& instr_type = get_instruction_type_table()[static_cast<uint8_t>(instr)];
EXPECT_EQ(instr_type.inputs.size(), 1);
EXPECT_EQ(instr_type.outputs.size(), 1);
EXPECT_EQ(instr_type.inputs[0], arg.type);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this check go inside lambda now?

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 test::execute() checks types inside.

@chfast
Copy link
Collaborator Author

chfast commented Jan 5, 2021

  1. I moved i64_eqz enhancement to test: Enhance the i64_eqz tests #679.
  2. Here we have one old commit which reworks the helpers + 2 commits updating the syntax of test cases. There can be squashed in various combinations if any preferences.
  3. The last WIP commit merges tests of the same instruction. I think it is fine doing it this way, but I don't have to proceed.

Base automatically changed from test_typed_result to master January 5, 2021 13:21
@chfast chfast merged commit 9ffd2d0 into master Jan 5, 2021
@chfast chfast deleted the test_op_executor branch January 5, 2021 14: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