This repository has been archived by the owner on Oct 28, 2021. It is now read-only.
testeth print test suite suggestions #5588
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gumb0
suggested changes
May 2, 2019
|
split into multiple lines? |
|
Not sure if making it like will help, maybe not. (I think I would try to do it at runtime with a visitor like here https://stackoverflow.com/a/8560992 seems simple enough) |
|
visitor requires global fixture that does not work if we use auto_test_fixture |
|
You could also make it an array of strings instead of one string, and concatenate or do anything you want with it at runtime, it should work. |
Codecov Report
@@ Coverage Diff @@
## master #5588 +/- ##
==========================================
+ Coverage 62.43% 62.51% +0.08%
==========================================
Files 350 350
Lines 29548 29648 +100
Branches 3326 3335 +9
==========================================
+ Hits 18448 18535 +87
- Misses 9898 9905 +7
- Partials 1202 1208 +6 |
chfast
suggested changes
May 6, 2019
gumb0
suggested changes
May 6, 2019
test/CMakeLists.txt
Outdated
| hunter_add_package(yaml-cpp) | ||
| find_package(yaml-cpp CONFIG REQUIRED) | ||
|
|
||
| add_executable(testeth ${sources}) | ||
| target_include_directories(testeth PRIVATE ${CMAKE_CURRENT_BINARY_DIR}) |
There was a problem hiding this comment.
this add a directory to which configure_file(tools/libtesteth/cmakeAllTests.h.in tools/libtesteth/cmakeAllTests.h) file is generated as include. its generated into build directory
winsvega
force-pushed
the
testeth
branch
2 times, most recently
from
484e23f to
6ed646c
Compare
|
updated the test name suggestion even better now ideally we could do a search in test name strings and increase the weight of a test name if there is search match. so for example if I type -t |
chfast
reviewed
May 9, 2019
|
I'm lost now in the cmake magic, what does |
gumb0
reviewed
May 9, 2019
It will be an array of string literals with names. They might be duplicated. |
|
I tried it and this is what it generates |
|
I don't really see duplicates there... Maybe if we put files with tests of the same test suite into different folders, test suite would be duplicated, not sure. |
gumb0
reviewed
May 9, 2019
Sorry for confusing comment. I meant that the CMake script generating this do not handle duplicates. But if they are full boost test names there should not be any as Boost would not allow to register 2 tests with the same name. Moreover, if you put all in a CMake list there is a builtin instruction to remove duplicates. |
|
cmake list now contain all tests and all test suites combined together. |
gumb0
approved these changes
May 10, 2019
|
I think it would be great to have a command that displays all test names, too (like |
|
there is boost cmd option |
chfast
approved these changes
May 15, 2019
|
Are those codecoves critical? |
|
You should add tests for the added code. https://codecov.io/gh/ethereum/aleth/compare/32f9c88f65fc30d7b682de651e2d40d8b7e26078...ff01d207fcefb7ed6ca037d215c51e5d66d76b90/diff |
chfast
reviewed
May 20, 2019
| BOOST_CHECK_EQUAL(distance, 6); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(getTestSuggestions) |
There was a problem hiding this comment.
This is not actually great unit tests because it depends on what tests are registered in other part of the project. I can leave with it, except this test does not pass.
There was a problem hiding this comment.
if it does not pass it means different behaviour depending on the build
gumb0
suggested changes
May 21, 2019
test/tools/libtesteth/TestHelper.h
Outdated
| @@ -131,6 +132,7 @@ json_spirit::mObject fillJsonWithStateChange(eth::State const& _stateOrig, eth:: | |||
| json_spirit::mObject fillJsonWithState(eth::State const& _state); | |||
| json_spirit::mObject fillJsonWithState(eth::State const& _state, eth::AccountMaskMap const& _map); | |||
| json_spirit::mObject fillJsonWithTransaction(eth::Transaction const& _txn); | |||
| std::vector<std::string> getTestSuggestions(std::string const& _sMinusTArg); | |||
There was a problem hiding this comment.
Suggested change
| std::vector<std::string> getTestSuggestions(std::string const& _sMinusTArg); | |
| std::vector<std::string> testSuggestions(std::string const& _sMinusTArg); |
|
|
||
| BOOST_AUTO_TEST_CASE(getTestSuggestions2) | ||
| { | ||
| // this return might change by more new tests get added |
There was a problem hiding this comment.
This is maintenance pain. Instead of using c_allTestNames in getTestSuggestions directly, you could pass it as a parameter, then pass the fake "all tests" here in tests.
There was a problem hiding this comment.
I would actually like to test that cmake constructed a valid test list array.
stExample2. Must find and suggest stExample test. I am planning to make it irrelevant with the order. Which somehow is different on the build
|
Also please add an item to CHANGELOG |
winsvega
force-pushed
the
testeth
branch
4 times, most recently
from
f28504b to
a69cfff
Compare
|
I have tried Andrey's suggestion to use tests list as a parametr. And found out that auto loop does not actually work with const char * (when its declared in the function arguments) |
gumb0
reviewed
May 27, 2019
test/tools/libtesteth/boostTest.cpp
Outdated
| #include <test/tools/jsontests/BlockChainTests.h> | ||
| #include <test/tools/jsontests/StateTests.h> | ||
| #include <test/tools/jsontests/TransactionTests.h> | ||
| #include <test/tools/jsontests/vm.h> | ||
| #include <test/tools/libtesteth/TestHelper.h> | ||
| #include <boost/test/included/unit_test.hpp> | ||
| #include <boost/tokenizer.hpp> |
test/tools/libtesteth/boostTest.cpp
Outdated
|
|
||
| void printTestSuiteSuggestions(string const& _sMinusTArg) | ||
| { | ||
| auto testList = test::testSuggestions(c_allTestNames, _sMinusTArg); |
| vector<string> const testList = { | ||
| "test1", "test2", "BlockSuite", "BlockSuite/TestCase", "GeneralBlockchainTests"}; | ||
| auto list = test::testSuggestions(testList, "blocksuit"); | ||
| BOOST_CHECK_EQUAL(test::inArray(list, string("BlockSuite")), true); |
There was a problem hiding this comment.
just BOOST_CHECK(inArray(list, string("BlockSuite"));
(also we have contains(...) in libdevcore/CommonData.h
| "GeneralBlockchainTests", "GeneralStateTests/stExample", "BCGeneralStateTests/stExample"}; | ||
|
|
||
| auto list = test::testSuggestions(testList, "GeneralStateTests/stExample2"); | ||
| BOOST_CHECK_EQUAL(test::inArray(list, string("GeneralStateTests/stExample")), true); |
CHANGELOG.md
Outdated
| @@ -9,6 +9,7 @@ | |||
| - Added: [#5575](https://github.com/ethereum/aleth/pull/5575) Log active peer count and peer list every 30 seconds. | |||
| - Added: [#5580](https://github.com/ethereum/aleth/pull/5580) Enable syncing from ETC nodes for blocks < dao hard fork block. | |||
| - Added: [#5591](https://github.com/ethereum/aleth/pull/5591) Network logging bugfixes and improvements and add p2pcap log channel. | |||
| - Added: [#5588](https://github.com/ethereum/aleth/pull/5588) Testeth print test suggestions | |||
There was a problem hiding this comment.
Suggested change
| - Added: [#5588](https://github.com/ethereum/aleth/pull/5588) Testeth print test suggestions | |
| - Added: [#5588](https://github.com/ethereum/aleth/pull/5588) Testeth prints similar test suite name suggestions, when the name passed in `-t` argument is not found. |
|
Hint: when modifying PR, that has already been reviewed, don't overwrite commits, but add new ones - this way it's easier for reviewers to see what you have changed. |
|
Fixed |
gumb0
approved these changes
May 29, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
when typing
./testeth -t <TestSuiteName>/<TestCaseName>in case of typo testeth will look on available test suites and print a suggestion