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

Display more informative messages by not rethrowing an exception #4375

Merged
merged 2 commits into from Aug 28, 2017

Conversation

Projects
None yet
4 participants
@pirapira
Member

pirapira commented Aug 22, 2017

This PR makes debugging easier when the fillers are malformed.

Before this PR:

75%...
100%
/home/yh/src/cpp-ethereum/test/tools/libtesteth/TestHelper.cpp(525): error: in "VMTests/vmtests": suicide Failed test with Exception: value type is 2 not 1
Test Case "vmArithmeticTest":
24%...

TestHelper.cpp(535) is not informative because it's a line that catches all errors.

After this PR:

50%...
75%...
100%
unknown location(0): fatal error: in "VMTests/vmtests": std::runtime_error: value type is 2 not 1
/home/yh/src/cpp-ethereum/test/tools/jsontests/vm.cpp(441): last checkpoint
Test Case "vmArithmeticTest":
24%...

vm.cpp(441), aha, it's a problem in "logs" field!

Both outputs are taken from

test/testeth -t 'VMTests' -- --filltests

@pirapira pirapira requested a review from chfast Aug 22, 2017

@pirapira pirapira requested a review from winsvega Aug 22, 2017

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Aug 22, 2017

Member

The new output does not contain the name of the test case, which is bad.

Member

pirapira commented Aug 22, 2017

The new output does not contain the name of the test case, which is bad.

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega Aug 22, 2017

Contributor

you could use TestOutputHelper to access the testcase name from different scopes.

Contributor

winsvega commented Aug 22, 2017

you could use TestOutputHelper to access the testcase name from different scopes.

@winsvega winsvega reopened this Aug 22, 2017

@pirapira pirapira removed request for chfast and winsvega Aug 22, 2017

@pirapira pirapira changed the title from Display more informative messages by not rethrowing an exception to [WIP] Display more informative messages by not rethrowing an exception Aug 22, 2017

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Aug 24, 2017

Codecov Report

Merging #4375 into develop will increase coverage by 0.01%.
The diff coverage is 82.35%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4375      +/-   ##
===========================================
+ Coverage    68.27%   68.28%   +0.01%     
===========================================
  Files          304      304              
  Lines        23212    23221       +9     
===========================================
+ Hits         15848    15857       +9     
  Misses        7364     7364
Impacted Files Coverage Δ
test/tools/libtesteth/TestHelper.cpp 49.5% <ø> (+0.32%) ⬆️
test/tools/jsontests/StateTests.cpp 94.89% <100%> (+0.27%) ⬆️
test/tools/jsontests/vm.cpp 64.49% <75%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae81a5b...8995948. Read the comment docs.

codecov-io commented Aug 24, 2017

Codecov Report

Merging #4375 into develop will increase coverage by 0.01%.
The diff coverage is 82.35%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4375      +/-   ##
===========================================
+ Coverage    68.27%   68.28%   +0.01%     
===========================================
  Files          304      304              
  Lines        23212    23221       +9     
===========================================
+ Hits         15848    15857       +9     
  Misses        7364     7364
Impacted Files Coverage Δ
test/tools/libtesteth/TestHelper.cpp 49.5% <ø> (+0.32%) ⬆️
test/tools/jsontests/StateTests.cpp 94.89% <100%> (+0.27%) ⬆️
test/tools/jsontests/vm.cpp 64.49% <75%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae81a5b...8995948. Read the comment docs.

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Aug 25, 2017

Member

@winsvega where shall I apply your suggestion? On top of this PR or on develop? Where am I supposed to access TestOutputHelper?

Member

pirapira commented Aug 25, 2017

@winsvega where shall I apply your suggestion? On top of this PR or on develop? Where am I supposed to access TestOutputHelper?

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega Aug 25, 2017

Contributor

you said you wanted more precise error information that shows the line of the error exception.
but you said thus if you remove exception it wont show you the test name.
so if you want to display a testname out of testsuite scope. use TestOutputHelper which stores the test name.

Contributor

winsvega commented Aug 25, 2017

you said you wanted more precise error information that shows the line of the error exception.
but you said thus if you remove exception it wont show you the test name.
so if you want to display a testname out of testsuite scope. use TestOutputHelper which stores the test name.

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Aug 25, 2017

Member

No, since I stopped catching the exception, I don't have an "out of testsuite" scope.

Member

pirapira commented Aug 25, 2017

No, since I stopped catching the exception, I don't have an "out of testsuite" scope.

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega Aug 25, 2017

Contributor

what is this comment about?
The new output does not contain the name of the test case, which is bad.

Contributor

winsvega commented Aug 25, 2017

what is this comment about?
The new output does not contain the name of the test case, which is bad.

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Aug 25, 2017

Member

This line

unknown location(0): fatal error: in "VMTests/vmtests": std::runtime_error: value type is 2 not 1
/home/yh/src/cpp-ethereum/test/tools/jsontests/vm.cpp(441): last checkpoint

does not contain the name of the test case. That's what I meant.

I don't have control over this printed message. I cannot use TestOutputHelper to improve this message, I think.

Member

pirapira commented Aug 25, 2017

This line

unknown location(0): fatal error: in "VMTests/vmtests": std::runtime_error: value type is 2 not 1
/home/yh/src/cpp-ethereum/test/tools/jsontests/vm.cpp(441): last checkpoint

does not contain the name of the test case. That's what I meant.

I don't have control over this printed message. I cannot use TestOutputHelper to improve this message, I think.

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega Aug 25, 2017

Contributor

well you could change boost require to bost_check_message on those lines.

Contributor

winsvega commented Aug 25, 2017

well you could change boost require to bost_check_message on those lines.

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Aug 25, 2017

Member

Good idea. Maybe before v.get_obj(), I can insert a boost_check_message.

[edit] I should do BoostRequireMessage.

Member

pirapira commented Aug 25, 2017

Good idea. Maybe before v.get_obj(), I can insert a boost_check_message.

[edit] I should do BoostRequireMessage.

pirapira added some commits Aug 22, 2017

Check types of JSON values
A JSON value might be an object, string, array or something else.  When get_obj() is called on a string, it causes an ugly error seen in
#4375

So this commit introduces checks before get_obj(), get_array() or get_str() is called.

@pirapira pirapira changed the title from [WIP] Display more informative messages by not rethrowing an exception to Display more informative messages by not rethrowing an exception Aug 28, 2017

@pirapira pirapira requested a review from winsvega Aug 28, 2017

@pirapira pirapira merged commit aac0843 into develop Aug 28, 2017

5 of 6 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
codecov/patch 82.35% of diff hit (target 50%)
Details
codecov/project 68.28% (+0.01%) compared to ae81a5b
Details
codecov/project/app 63.05% remains the same compared to ae81a5b
Details
codecov/project/tests 77.83% (+0.02%) compared to ae81a5b
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@pirapira pirapira deleted the informative branch Aug 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment