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

State diff #4074

Merged
merged 4 commits into from May 26, 2017

Conversation

Projects
None yet
5 participants
@winsvega
Contributor

winsvega commented May 11, 2017

add --statediff option when filling or running tests

@winsvega winsvega requested review from gumb0 and chriseth May 11, 2017

@gumb0 gumb0 added needs review and removed in progress labels May 11, 2017

@gumb0 gumb0 requested a review from chfast May 11, 2017

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io May 12, 2017

Codecov Report

Merging #4074 into develop will decrease coverage by 0.27%.
The diff coverage is 24.84%.

@@             Coverage Diff             @@
##           develop    #4074      +/-   ##
===========================================
- Coverage     65.3%   65.03%   -0.28%     
===========================================
  Files          308      309       +1     
  Lines        22897    23000     +103     
===========================================
+ Hits         14954    14959       +5     
- Misses        7943     8041      +98
Impacted Files Coverage Δ
test/tools/libtesteth/ImportTest.h 100% <ø> (ø) ⬆️
test/tools/libtesteth/Options.h 100% <ø> (ø) ⬆️
test/tools/libtesteth/TestHelper.h 50% <ø> (ø) ⬆️
libethereum/State.h 94.28% <ø> (ø) ⬆️
libethereum/Executive.cpp 57.43% <0%> (ø) ⬆️
test/tools/libtesteth/FillJsonFunctions.cpp 20.56% <20.56%> (ø)
test/tools/libtesteth/ImportTest.cpp 49.1% <25.71%> (-2.68%) ⬇️
test/tools/libtesteth/Options.cpp 30.49% <33.33%> (+0.06%) ⬆️
test/tools/libtesteth/TestHelper.cpp 51.25% <50%> (-1.16%) ⬇️
test/tools/jsontests/StateTests.cpp 95.45% <50%> (-1.06%) ⬇️
... and 8 more

codecov-io commented May 12, 2017

Codecov Report

Merging #4074 into develop will decrease coverage by 0.27%.
The diff coverage is 24.84%.

@@             Coverage Diff             @@
##           develop    #4074      +/-   ##
===========================================
- Coverage     65.3%   65.03%   -0.28%     
===========================================
  Files          308      309       +1     
  Lines        22897    23000     +103     
===========================================
+ Hits         14954    14959       +5     
- Misses        7943     8041      +98
Impacted Files Coverage Δ
test/tools/libtesteth/ImportTest.h 100% <ø> (ø) ⬆️
test/tools/libtesteth/Options.h 100% <ø> (ø) ⬆️
test/tools/libtesteth/TestHelper.h 50% <ø> (ø) ⬆️
libethereum/State.h 94.28% <ø> (ø) ⬆️
libethereum/Executive.cpp 57.43% <0%> (ø) ⬆️
test/tools/libtesteth/FillJsonFunctions.cpp 20.56% <20.56%> (ø)
test/tools/libtesteth/ImportTest.cpp 49.1% <25.71%> (-2.68%) ⬇️
test/tools/libtesteth/Options.cpp 30.49% <33.33%> (+0.06%) ⬆️
test/tools/libtesteth/TestHelper.cpp 51.25% <50%> (-1.16%) ⬇️
test/tools/jsontests/StateTests.cpp 95.45% <50%> (-1.06%) ⬇️
... and 8 more
@@ -629,6 +654,10 @@ int ImportTest::exportTest(bytes const& _output)
if (it != std::end(stateIndexesToPrint))
obj2["postState"] = fillJsonWithState(m_transactions[i].postState);
}
if (Options::get().statediff)

This comment has been minimized.

@gumb0

gumb0 May 15, 2017

Member

Does this go into generated json files?

@gumb0

gumb0 May 15, 2017

Member

Does this go into generated json files?

This comment has been minimized.

@winsvega

winsvega May 15, 2017

Contributor

when using with --filltests yes.

@winsvega

winsvega May 15, 2017

Contributor

when using with --filltests yes.

This comment has been minimized.

@gumb0

gumb0 May 15, 2017

Member

Do we really need to write it into files and incerase their size and complexity? It's only for human readers, right?

Maybe just leave it as a log option for testeth run ?

@gumb0

gumb0 May 15, 2017

Member

Do we really need to write it into files and incerase their size and complexity? It's only for human readers, right?

Maybe just leave it as a log option for testeth run ?

This comment has been minimized.

@chfast

chfast May 24, 2017

Collaborator

Answer for this?

@chfast

chfast May 24, 2017

Collaborator

Answer for this?

@gumb0

This comment has been minimized.

Show comment
Hide comment
@gumb0

gumb0 May 15, 2017

Member

It probably would be nice to have this feature not only for tests, but also for regular eth operation, like we have vmtrace for both... Would it be difficult to output state diff to log in eth when, say, VMTRACE is enabled ?

Member

gumb0 commented May 15, 2017

It probably would be nice to have this feature not only for tests, but also for regular eth operation, like we have vmtrace for both... Would it be difficult to output state diff to log in eth when, say, VMTRACE is enabled ?

@chfast

This comment has been minimized.

Show comment
Hide comment
@chfast

chfast May 15, 2017

Collaborator

Ok, let's take is slowly.
Can we separate the changes related to toCompactHex from the "state diff" one first?

Collaborator

chfast commented May 15, 2017

Ok, let's take is slowly.
Can we separate the changes related to toCompactHex from the "state diff" one first?

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega May 17, 2017

Contributor

refactored the log function.
I've removed changeLog clean function from commit in state. and moved it to the begining of execute.
will see how tests work. if there is an issue we could dublicate m_changeLog variable in State just for getChangeLog() method

Contributor

winsvega commented May 17, 2017

refactored the log function.
I've removed changeLog clean function from commit in state. and moved it to the begining of execute.
will see how tests work. if there is an issue we could dublicate m_changeLog variable in State just for getChangeLog() method

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira May 22, 2017

Member

I think ChangLog.md can talk about the new option.

Member

pirapira commented May 22, 2017

I think ChangLog.md can talk about the new option.

Show outdated Hide outdated libethereum/StateDetail.h
@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega May 24, 2017

Contributor

the first solution was to put changeLog into execution results. that required separation of StateDetail from State

Contributor

winsvega commented May 24, 2017

the first solution was to put changeLog into execution results. that required separation of StateDetail from State

You should have received a copy of the GNU General Public License
along with cpp-ethereum. If not, see <http://www.gnu.org/licenses/>.
*/
/** @file

This comment has been minimized.

@chfast

chfast May 24, 2017

Collaborator

What is the point of not having these function in TestHelper.cpp?

@chfast

chfast May 24, 2017

Collaborator

What is the point of not having these function in TestHelper.cpp?

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega May 24, 2017

Contributor

to split the large file with lots of functions into smaller ones

Contributor

winsvega commented May 24, 2017

to split the large file with lots of functions into smaller ones

@chfast

This comment has been minimized.

Show comment
Hide comment
@chfast

chfast May 24, 2017

Collaborator

This just makes it compile twice as long as before.

Collaborator

chfast commented May 24, 2017

This just makes it compile twice as long as before.

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega May 24, 2017

Contributor

I stick to that guy's opinion:
"f I find that an implementation file is thousands of lines long, that's usually a sign that there's too much there and I need to break it up."
https://stackoverflow.com/questions/531133/should-i-put-many-functions-into-one-file-or-more-or-less-one-function-per-fi

Contributor

winsvega commented May 24, 2017

I stick to that guy's opinion:
"f I find that an implementation file is thousands of lines long, that's usually a sign that there's too much there and I need to break it up."
https://stackoverflow.com/questions/531133/should-i-put-many-functions-into-one-file-or-more-or-less-one-function-per-fi

@chfast

Can you also add a documentation with examples how to use it?
You should move testeth documentation to docs dir in this repo.

Show outdated Hide outdated libethereum/State.cpp
Show outdated Hide outdated libethereum/State.cpp
Show outdated Hide outdated libethereum/State.cpp
@@ -629,6 +654,10 @@ int ImportTest::exportTest(bytes const& _output)
if (it != std::end(stateIndexesToPrint))
obj2["postState"] = fillJsonWithState(m_transactions[i].postState);
}
if (Options::get().statediff)

This comment has been minimized.

@chfast

chfast May 24, 2017

Collaborator

Answer for this?

@chfast

chfast May 24, 2017

Collaborator

Answer for this?

Show outdated Hide outdated test/tools/libtesteth/TestHelper.cpp
@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega May 24, 2017

Contributor

staediff also work when filling tests producing output to the test .json for debug

Contributor

winsvega commented May 24, 2017

staediff also work when filling tests producing output to the test .json for debug

Show outdated Hide outdated libethereum/State.cpp
Show outdated Hide outdated test/tools/libtesteth/ImportTest.cpp
Show outdated Hide outdated libethereum/State.cpp
Show outdated Hide outdated libethcore/ChainOperationParams.h
Show outdated Hide outdated libethereum/State.cpp
Show outdated Hide outdated libethereum/State.cpp
Show outdated Hide outdated libethereum/State.h
Show outdated Hide outdated test/tools/libtesteth/ImportTest.cpp
Show outdated Hide outdated test/tools/libtesteth/ImportTest.cpp
Show outdated Hide outdated test/tools/libtesteth/ImportTest.h
Show outdated Hide outdated test/tools/libtesteth/ImportTest.h
@@ -477,7 +422,9 @@ void executeTests(const string& _name, const string& _testPathAppendix, const st
}
try
{
cnote << "TEST " << name << ":";
if ((Options::get().singleTest && Options::get().singleTestName == name) || !Options::get().singleTest)
cnote << "TEST " << name << ":";

This comment has been minimized.

@chfast

chfast May 26, 2017

Collaborator

Verbosity 2 outputs test names. Why do you need this?

@chfast

chfast May 26, 2017

Collaborator

Verbosity 2 outputs test names. Why do you need this?

This comment has been minimized.

@winsvega

winsvega May 26, 2017

Contributor

so that when you use --statediff you would see only the name of the test you are running with --singletest option.
other names make it hard to see the changeLog in console when there are many tests

@winsvega

winsvega May 26, 2017

Contributor

so that when you use --statediff you would see only the name of the test you are running with --singletest option.
other names make it hard to see the changeLog in console when there are many tests

Show outdated Hide outdated libethereum/State.cpp
Show outdated Hide outdated libethereum/State.h
Show outdated Hide outdated libethereum/State.h
LogEntries emptyLogs;
ExecutionResult emptyRes;
ImportTest::ExecOutput execOut = make_pair(emptyRes, TransactionReceipt(h256(), u256(), emptyLogs));
std::tie(m_statePost, execOut, std::ignore) =

This comment has been minimized.

@chfast

chfast May 26, 2017

Collaborator

Here this change was not necessary as ExecutionResult does not have default constructor, but it can stay.

@chfast

chfast May 26, 2017

Collaborator

Here this change was not necessary as ExecutionResult does not have default constructor, but it can stay.

This comment has been minimized.

@winsvega

winsvega May 26, 2017

Contributor

we will remove this part anyway at some point

@winsvega

winsvega May 26, 2017

Contributor

we will remove this part anyway at some point

Show outdated Hide outdated libethereum/State.h
Show outdated Hide outdated test/tools/libtesteth/FillJsonFunctions.cpp
@chfast

chfast approved these changes May 26, 2017

@winsvega winsvega merged commit 4f9ff4a into ethereum:develop May 26, 2017

2 of 4 checks passed

codecov/patch 24.84% of diff hit (target 80%)
Details
codecov/project 65.03% (-0.28%) compared to 8576453
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@winsvega winsvega removed the needs review label May 26, 2017

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