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

testeth: add stTimeConsuming #5576

Merged
merged 2 commits into from
Apr 29, 2019
Merged

testeth: add stTimeConsuming #5576

merged 2 commits into from
Apr 29, 2019

Conversation

winsvega
Copy link
Contributor

this tests take more then 3 minutes of test execution time.
also PR updates the test submodule head

@winsvega winsvega requested a review from gumb0 April 24, 2019 14:06
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 an item to CHANGELOG.md stating which tests will not run by default now

@@ -147,7 +147,8 @@ class GeneralTestFixture
boost::filesystem::path suiteFillerPath = suite.getFullPathFiller(casename).parent_path();

// Check specific test cases
static vector<string> timeConsumingTestSuites{string{"stQuadraticComplexityTest"}};
static vector<string> timeConsumingTestSuites{
Copy link
Member

Choose a reason for hiding this comment

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

can be const

@gumb0
Copy link
Member

gumb0 commented Apr 24, 2019

cc @chfast

@chfast
Copy link
Member

chfast commented Apr 24, 2019

I don't understand the direction this is going. Is it safe to skip these tests on regular basis? I'd rather check why they take so much time, maybe we can reduce the number of them, etc. If you disable them, 99% of users (including us) will not run them.

If you moved e.g. stSstore tests, does it mean we don't test (unless explicitly enabled) one of the features at all?

@winsvega
Copy link
Contributor Author

winsvega commented Apr 25, 2019

Only those tests I moved which take a lot of time to execute.

To run all tests you anyway have to run with --all flag

@chfast
Copy link
Member

chfast commented Apr 25, 2019

Only those tests I moved which take a lot of time to execute.

You mean test suites or test cases?

@winsvega
Copy link
Contributor Author

Test cases. One specific test is 50000 sha256 execution.
And 5000 of sstore combinations tests

@chfast
Copy link
Member

chfast commented Apr 25, 2019

Ok, that's fine then.

One specific test is 50000 sha256 execution.

Are you sure such tests are useful? I believe you get the same effect when running sha256 10x instead of 50000.

@winsvega
Copy link
Contributor Author

Are you sure such tests are useful? I believe you get the same effect when running sha256 10x instead of 50000.

don't remember why exactly Christoph added this test case. but I see its potential to crush some clients or make it timeout due to not optimized coding.

@codecov-io
Copy link

Codecov Report

Merging #5576 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5576      +/-   ##
==========================================
- Coverage   62.11%   62.11%   -0.01%     
==========================================
  Files         347      347              
  Lines       29107    29103       -4     
  Branches     3302     3299       -3     
==========================================
- Hits        18080    18077       -3     
+ Misses       9847     9843       -4     
- Partials     1180     1183       +3

@winsvega
Copy link
Contributor Author

net tests failing randomly

- Changed: [#5568](https://github.com/ethereum/aleth/pull/5568) Improve rlpx handshake log messages and create new rlpx log channel.
- Changed: [#5576](https://github.com/ethereum/aleth/pull/5576) Moved sstore_combinations and static_Call50000_sha256 tests to stTimeConsuming test suite. (testeth runs them only with `--all` flag)
Copy link
Member

Choose a reason for hiding this comment

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

(see https://keepachangelog.com/en/1.0.0/#how for changelog format)

@gumb0
Copy link
Member

gumb0 commented Apr 29, 2019

net tests failing randomly

We know, see #5544 and #5581

@winsvega winsvega merged commit 7112405 into master Apr 29, 2019
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