Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

support LegacyTests in testeth #5735

Merged
merged 8 commits into from
Sep 12, 2019
Merged

support LegacyTests in testeth #5735

merged 8 commits into from
Sep 12, 2019

Conversation

winsvega
Copy link
Contributor

@winsvega winsvega commented Sep 4, 2019

update the test subrepo to latest develop
support testeth to run LegacyTests (all tests for forks <= Constantinople. inc. GeneralStateTests and its blockchain form at the moment)

./testeth -t LegacyTests -- --all

note this tests only run on linux-gcc6 yaml because --all option is enabled

@winsvega winsvega requested a review from gumb0 September 4, 2019 15:18
Copy link
Member

@gumb0 gumb0 left a comment

Choose a reason for hiding this comment

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

Some tests are failing

@codecov-io
Copy link

codecov-io commented Sep 4, 2019

Codecov Report

Merging #5735 into master will increase coverage by 0.31%.
The diff coverage is 95.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5735      +/-   ##
==========================================
+ Coverage   63.35%   63.67%   +0.31%     
==========================================
  Files         353      356       +3     
  Lines       30352    30469     +117     
  Branches     3395     3390       -5     
==========================================
+ Hits        19230    19401     +171     
+ Misses       9886     9843      -43     
+ Partials     1236     1225      -11

test::TestOutputHelper::get().markTestFolderAsFinished(suiteFillerPath, casename);
return;
}
suite.runAllTestsInFolder(casename);
Copy link
Member

Choose a reason for hiding this comment

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

this is just a rant, we won't change it now, but this should have been called from the test body instead of weirdly from fixture constructor; for that you could have created a macro wrapping BOOST_AUTO_TEST_CASE macro always calling it from the body
(maybe do that in retesteth if it's any similar there)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please explain better the idea.

Copy link
Member

@gumb0 gumb0 Sep 9, 2019

Choose a reason for hiding this comment

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

Don't run the tests from the constructor, run them from the test body.

#define RETESTETH_AUTO_TEST_CASE(casename) BOOST_AUTO_TEST_CASE(casename) { m_suite.runAllTestsInFolder(casename); }
RETESTETH_AUTO_TEST_CASE(stCallCodes) 

...anyway it's better to get rid of boost.test altogether

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it will get more confusing if to continue refactoring it that way

@gumb0
Copy link
Member

gumb0 commented Sep 6, 2019

I don't see legacy tests running on AppVeyor, do they run? (maybe just the suite name is not logged?)

@gumb0
Copy link
Member

gumb0 commented Sep 6, 2019

Add item to changelog

@winsvega
Copy link
Contributor Author

winsvega commented Sep 6, 2019

In AppVeyor it just runs testeth that output just the casenames without the tree structure.
The tests get executed. (no --all flag is set )

Copy link
Member

@gumb0 gumb0 left a comment

Choose a reason for hiding this comment

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

Add item to changelog

test/tools/jsontests/Common.h Outdated Show resolved Hide resolved
test/tools/jsontests/Common.h Outdated Show resolved Hide resolved
test/tools/jsontests/Common.h Outdated Show resolved Hide resolved
test/tools/jsontests/LegacyTestsBoost.cpp Outdated Show resolved Hide resolved
test/tools/jsontests/Common.h Outdated Show resolved Hide resolved
test/tools/jsontests/Common.h Outdated Show resolved Hide resolved
test/tools/jsontests/Common.h Outdated Show resolved Hide resolved
test/tools/jsontests/Common.h Outdated Show resolved Hide resolved
@winsvega winsvega requested a review from gumb0 September 10, 2019 15:18
Copy link
Member

@gumb0 gumb0 left a comment

Choose a reason for hiding this comment

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

more comments

CHANGELOG.md Outdated Show resolved Hide resolved
test/tools/jsontests/LegacyTestsBoost.cpp Outdated Show resolved Hide resolved
test/tools/jsontests/LegacyTestsBoost.h Outdated Show resolved Hide resolved
test/tools/jsontests/StateTestFixtureBase.h Outdated Show resolved Hide resolved
test/tools/libtesteth/TestHelper.cpp Outdated Show resolved Hide resolved
Copy link
Member

@gumb0 gumb0 left a comment

Choose a reason for hiding this comment

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

looks better now, a couple of small things left

test/tools/jsontests/StateTestFixtureBase.h Outdated Show resolved Hide resolved
test/tools/jsontests/StateTestFixtureBase.h Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@gumb0
Copy link
Member

gumb0 commented Sep 11, 2019

LegacyTests/Constantinople/BCGeneralStateTests are reported as passed by ctest, but they are actually skipped, aren't they?
But it was similar before for blockchain tests generated from state tests, right?

@winsvega
Copy link
Contributor Author

winsvega commented Sep 11, 2019

LegacyTests/Constantinople/BCGeneralStateTests get executed if ctest run testeth with --all flag. otherwise it is skipped same as stTimeConsuming tests.

I've checked the scripts. I think it was macOS that runs all tests with --all flag.

Copy link
Member

@gumb0 gumb0 left a comment

Choose a reason for hiding this comment

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

Please squash at least first 4 commits, to make the anonymous ones disappear

@winsvega
Copy link
Contributor Author

is there a command to squash commits in the history not touching the recent ones?

@gumb0
Copy link
Member

gumb0 commented Sep 11, 2019

depends on what you mean by not touching
git rebase -i HEAD~9 and set the first ones to squash and the last ones to pick
(all commits will be rewritten anyway, resulting in new commit hashes)

@winsvega
Copy link
Contributor Author

winsvega commented Sep 11, 2019

looks like I can't sqush the first unnamed commit. you could only squash following commits into previous.

check it out on branch: legacytests_squash
if its ok. I will push it to this branch.

@gumb0
Copy link
Member

gumb0 commented Sep 12, 2019

That branch is ok, it shows first commit to be authored by Wins and committed by @winsvega

Now GCC compiler is crashing, could you also cherry-pick this commit here 874a5e7

@winsvega
Copy link
Contributor Author

have you just squashed the commits in this branch ok?

@gumb0
Copy link
Member

gumb0 commented Sep 12, 2019

Yes, it's fine here now

@gumb0
Copy link
Member

gumb0 commented Sep 12, 2019

@chfast Is it ok to merge this with BUILD_PARALLEL_JOBS: 2 for GCC?

@chfast
Copy link
Member

chfast commented Sep 12, 2019

@chfast Is it ok to merge this with BUILD_PARALLEL_JOBS: 2 for GCC?

Yes

@gumb0 gumb0 merged commit 68e273d into master Sep 12, 2019
@gumb0 gumb0 deleted the legacytests branch September 12, 2019 13:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants