test: Pass ArgsManager into getarg_tests #18926
pull glowang wants to merge 2 commits into bitcoin:master from glowang:replace_global_gArgs_with_local_argsman changing 1 files +71 −62-
glowang commented at 10:39 pm on May 9, 2020: contributorReplaced the global argsManager gArgs with a locally defined one in getarg_tests. This is to avoid confusion in arg settings between the test’s ArgsManager and the #18804
-
fanquake added the label Tests on May 9, 2020
-
fanquake requested review from ryanofsky on May 10, 2020
-
glowang renamed this:
Pass ArgsManager into functions that register args with AddArg
Pass ArgsManager into getarg_tests
on May 10, 2020 -
glowang marked this as ready for review on May 10, 2020
-
glowang force-pushed on May 12, 2020
-
glowang force-pushed on May 12, 2020
-
in src/test/getarg_tests.cpp:42 in af069fcdd9 outdated
29@@ -30,7 +30,7 @@ static void ResetArgs(const std::string& strArg) 30 vecChar.push_back(s.c_str()); 31 32 std::string error; 33- BOOST_CHECK(gArgs.ParseParameters(vecChar.size(), vecChar.data(), error)); 34+ BOOST_CHECK(argsman.ParseParameters(vecChar.size(), vecChar.data(), error));
ryanofsky commented at 5:29 pm on May 12, 2020:In commit “scripted-diff: replace gArgs with a local args manager” (af069fcdd9b5e2c983ee32f0292c8e1f167359ec)
In the commit message for https://github.com/bitcoin/bitcoin/commit/af069fcdd9b5e2c983ee32f0292c8e1f167359ec.patch you need an extra newline between the subject and body of the commit for the scripted diff to work like:
0scripted-diff: replace gArgs with a local args manager 1 2-BEGIN VERIFY SCRIPT- 3sed -i -e '205!s/gArgs/argsman/g' src/test/getarg_tests.cpp 4-END VERIFY SCRIPT-
glowang commented at 0:26 am on May 13, 2020:Thank you! I will update the developer README too then. -
in src/test/getarg_tests.cpp:36 in 2e0c74b1ff outdated
32@@ -33,6 +33,8 @@ static void ResetArgs(const std::string& strArg) 33 BOOST_CHECK(gArgs.ParseParameters(vecChar.size(), vecChar.data(), error)); 34 } 35 36+ArgsManager argsman;
ryanofsky commented at 5:35 pm on May 12, 2020:In commit “Include an unused ArgsManager” (2e0c74b1ffe9bd1867700fd4a58458160324e23c)
This variable is still shared between test instances (and I think also accesible outside the test file using
extern
). I think it would be better to add a member variable to the boost test setup so the tests are isolated. This would look something likehttps://github.com/ryanofsky/bitcoin/commit/db762529d6bffcc6ab0c1c64b9378925f2d75fa1
glowang commented at 3:57 pm on May 13, 2020:Thank you, will fix! -
ryanofsky approved
-
ryanofsky commented at 5:39 pm on May 12, 2020: memberCode review ACK af069fcdd9b5e2c983ee32f0292c8e1f167359ec. This change seems good and is an improvement, but I left a suggestion to use a unique ArgsManager for each test case instead of a shared one.
-
glowang force-pushed on May 13, 2020
-
in doc/developer-notes.md:963 in a2efd7758e outdated
959@@ -960,6 +960,7 @@ To create a scripted-diff: 960 - in the commit message include the bash script between lines containing just the following text: 961 - `-BEGIN VERIFY SCRIPT-` 962 - `-END VERIFY SCRIPT-` 963+- note: you need an extra newline between the above two steps for the scripted diff to work properly
MarcoFalke commented at 5:21 pm on May 13, 2020:The newline is generally needed for git to work properly. This is already explained in CONTRIBUTING, see this section:
0Commit messages should be verbose by default consisting of a short subject line 1(50 chars max), a blank line and detailed explanatory text as separate 2paragraph(s), unless the title alone is self-explanatory (like "Corrected typo 3in init.cpp") in which case a single title line is sufficient. Commit messages should be 4helpful to people reading your code in the future, so explain the reasoning for 5your decisions. Further explanation [here](https://chris.beams.io/posts/git-commit/).
-
MarcoFalke commented at 5:22 pm on May 13, 2020: memberConcept ACK. Thanks for switching the tests away from using
gArgs
this could badly interfere with later tests and cause hard to debug run-time issues -
MarcoFalke referenced this in commit fa1552b6ef on May 14, 2020
-
glowang force-pushed on May 15, 2020
-
glowang force-pushed on May 16, 2020
-
in src/test/getarg_tests.cpp:16 in c5ee23990e outdated
12@@ -13,10 +13,17 @@ 13 #include <boost/algorithm/string.hpp> 14 #include <boost/test/unit_test.hpp> 15 16-BOOST_FIXTURE_TEST_SUITE(getarg_tests, BasicTestingSetup) 17+class LocalTestingSetup : BasicTestingSetup {
glowang commented at 4:40 pm on May 16, 2020:Note to self - I kept gettingcannot define or redeclare 'ResetArgs' here because namespace 'getarg_tests' does not enclose namespace 'LocalTestingSetup'
MarcoFalke commented at 4:43 pm on May 16, 2020:I kept getting cannot define or redeclare ‘ResetArgs’ here because namespace ‘getarg_tests’ does not enclose namespace ‘LocalTestingSetup’
You can put this into a namespace. Like in
validation_block_tests
-
glowang force-pushed on May 19, 2020
-
in src/test/getarg_tests.cpp:200 in 03ca212cec outdated
199@@ -200,7 +200,7 @@ BOOST_AUTO_TEST_CASE(logargs) 200 const auto okaylog_negbool = std::make_pair("-okaylog-negbool", ArgsManager::ALLOW_BOOL); 201 const auto okaylog = std::make_pair("-okaylog", ArgsManager::ALLOW_ANY); 202 const auto dontlog = std::make_pair("-dontlog", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE);
glowang commented at 5:27 am on May 19, 2020:test/getarg_tests.cpp:218: error: in “getarg_tests/logargs”: check str.find(“Command-line arg: okaylog-bool=""”) != std::string::npos has failed test/getarg_tests.cpp:219: error: in “getarg_tests/logargs”: check str.find(“Command-line arg: okaylog-negbool=false”) != std::string::npos has failed test/getarg_tests.cpp:220: error: in “getarg_tests/logargs”: check str.find(“Command-line arg: okaylog="public"”) != std::string::npos has failed test/getarg_tests.cpp:221: error: in “getarg_tests/logargs”: check str.find(“dontlog=****”) != std::string::npos has failed
My tests passed in the first 2 commits, but this commit gives me these errors locally. It looks like the bug comes from my
debug.log
having something that it should not. My struggle is I can’t really find my debug log…I checked out some resources (Andrew Chow’s/bitcoin wiki), but none seems to mention where to locate this file.https://gist.github.com/fjahr/2cd23ad743a2ddfd4eed957274beca0f does include an example command
$ cat ~/Library/Application\ Support/Bitcoin/regtest/debug.log
but this doesn’t work for me locally. Any tips? @amitiuttarwar @fjahr 🙏
ryanofsky commented at 10:43 am on May 19, 2020:re: #18926 (review)
My tests passed in the first 2 commits, but this commit gives me these errors locally. It looks like the bug comes from my
debug.log
having something that it should not.That’s right, the problem is happening because the arguments are being set in the
argman
instance notgArgs
but line 214 still saysgArgs.LogArgs()
notargsman.LogArgs()
.My struggle is I can’t really find my debug log…I checked out some resources (Andrew Chow’s/bitcoin wiki), but none seems to mention where to locate this file.
I believe it’s written to a temporary directory so you could check TMPDIR, but most convenient way to see logs from tests is:
0src/test/test_bitcoin -l test_suite -t getarg_tests -- DEBUG_LOG_OUT
Also, in my previous comment #18926 (review), I did link to a working updated copy of the test. If you want to apply and test those changes locally you can do that with:
0git fetch https://github.com/ryanofsky/bitcoin review.18926.1-edit 1git checkout FETCH_HEAD src/test/getarg_tests.cpp 2make -C src test/test_bitcoin 3src/test/test_bitcoin -l test_suite -t getarg_tests -- DEBUG_LOG_OUT
glowang commented at 2:23 pm on May 19, 2020:ahhhh thank you! I avoided line 214 in my sed command, because it would modify
LogArgs()
toLoargsman()
, but I overlooked the use of the global manager in there. I will look what sed command I can use to avoid changing only a specific location of line.I feel like if I commited your solution, my scripted-diff commit on its own is still failing the test…this makes my commits depend on each other, instead of working modularly.
MarcoFalke commented at 2:29 pm on May 19, 2020:You can use
0$ echo 'gArgs.LogArgs()' | sed -e 's/\<gArgs\>/____/g' 1____.LogArgs()
ryanofsky commented at 3:36 pm on May 19, 2020:I feel like if I commited your solution
To be sure, suggestion was just to compare against for debugging, not to commit
glowang commented at 3:29 pm on May 22, 2020:You can use
0$ echo 'gArgs.LogArgs()' | sed -e 's/\<gArgs\>/____/g' 1____.LogArgs()
Thanks Marco, I tried running this but it is not changing anything from my end. Is there any setup I needed to do? Are you using gnu-sed? This is what I tried.
sed -i -e 's/\<gArgs\>/argsman/g' src/test/getarg_tests.cpp
I did receive a similar suggestion from my post on S/O, but somehow mine is not working 😂
glowang commented at 3:29 pm on May 22, 2020:I feel like if I commited your solution
To be sure, suggestion was just to compare against for debugging, not to commit
Totally. Thanks again for your suggestion :)
glowang commented at 3:51 pm on May 22, 2020:You can use
0$ echo 'gArgs.LogArgs()' | sed -e 's/\<gArgs\>/____/g' 1____.LogArgs()
Thanks Marco, I tried running this but it is not changing anything from my end. Is there any setup I needed to do? Are you using gnu-sed? This is what I tried.
sed -i -e 's/\<gArgs\>/argsman/g' src/test/getarg_tests.cpp
I did receive a similar suggestion from my post on S/O, but somehow mine is not working 😂
Maybe I should look more into the regex itself actually
MarcoFalke commented at 3:53 pm on May 22, 2020:Yes, you will need to use gsed. Our scripted diffs don’t support macos sed, only Ubuntu sed. -
in src/test/getarg_tests.cpp:20 in 52a4cdfc18 outdated
12@@ -13,10 +13,19 @@ 13 #include <boost/algorithm/string.hpp> 14 #include <boost/test/unit_test.hpp> 15 16-BOOST_FIXTURE_TEST_SUITE(getarg_tests, BasicTestingSetup) 17+namespace getarg_tests{ 18+ class LocalTestingSetup : BasicTestingSetup { 19+ protected: 20+ void SetupArgs(ArgsManager& m_args, const std::vector<std::pair<std::string, unsigned int>>& args); 21+ void ResetArgs(const std::string& strArg);
dongcarl commented at 10:11 pm on May 21, 2020:Might wanna remove these trailing whitespaces to appease Travis
glowang commented at 3:29 pm on May 22, 2020:Thanks Carl 🙏 -
glowang force-pushed on May 23, 2020
-
glowang force-pushed on May 23, 2020
-
glowang force-pushed on May 23, 2020
-
glowang requested review from MarcoFalke on May 23, 2020
-
glowang requested review from ryanofsky on May 23, 2020
-
glowang requested review from dongcarl on May 23, 2020
-
glowang commented at 6:11 pm on May 23, 2020: contributorThis is ready for re-review.
-
MarcoFalke renamed this:
Pass ArgsManager into getarg_tests
test: Pass ArgsManager into getarg_tests
on May 23, 2020 -
in src/test/getarg_tests.cpp:21 in af01fdb9f8 outdated
17+namespace getarg_tests{ 18+ class LocalTestingSetup : BasicTestingSetup { 19+ protected: 20+ void SetupArgs(ArgsManager& m_args, const std::vector<std::pair<std::string, unsigned int>>& args); 21+ void ResetArgs(const std::string& strArg); 22+ ArgsManager argsman;
MarcoFalke commented at 8:48 pm on May 23, 2020:0 ArgsManager m_args;
according to the dev notes on member naming
(the scripted diff needs to be changed to m_args as well, obviously)
-
in src/test/getarg_tests.cpp:19 in af01fdb9f8 outdated
12@@ -13,9 +13,18 @@ 13 #include <boost/algorithm/string.hpp> 14 #include <boost/test/unit_test.hpp> 15 16-BOOST_FIXTURE_TEST_SUITE(getarg_tests, BasicTestingSetup) 17+namespace getarg_tests{ 18+ class LocalTestingSetup : BasicTestingSetup { 19+ protected: 20+ void SetupArgs(ArgsManager& m_args, const std::vector<std::pair<std::string, unsigned int>>& args);
MarcoFalke commented at 8:50 pm on May 23, 2020:0 void SetupArgs(const std::vector<std::pair<std::string, unsigned int>>& args);
This should be removed (along with the first commit), because in a member function you don’t need to pass in member variables anymore
-
MarcoFalke commented at 8:50 pm on May 23, 2020: memberAlmost-ACK
-
Create a local class inherited from BasicTestingSetup with a localized args manager
and put it into the getarg_tests namespace
-
scripted-diff: replace gArgs with argsman
-BEGIN VERIFY SCRIPT- sed -i 's/\<gArgs\>/m_args/g' src/test/getarg_tests.cpp -END VERIFY SCRIPT-
-
glowang force-pushed on May 28, 2020
-
MarcoFalke commented at 1:39 pm on May 28, 2020: memberACK f871f15c9d760b56a6a3b78b3941d3983d60810c
-
ryanofsky approved
-
ryanofsky commented at 5:53 pm on May 29, 2020: member
Code review ACK f871f15c9d760b56a6a3b78b3941d3983d60810c. Changes look good and thanks for updating. In future would recommend using clang-format-diff and following coding style notes, because it’s atypical to indent namespace content, or indent protected keywords or put spaces around ::. Also it’s fragile to define test setup class in a namespace, but test setup methods outside of the namespace and inside the test fixture instead. Would be simpler to just define the testing setup completely before using it without a namespace like: https://github.com/bitcoin/bitcoin/blob/8ad5f1c376fe99eeccac2696ac0e18e662323a4d/src/test/rpc_tests.cpp#L23 and it would have been a slightly smaller change too.
No need to change anything though, this is already better
-
MarcoFalke commented at 6:25 pm on May 29, 2020: memberIf someone want to fix the “namespace issue” in a follow-up, the same should probably be done in the other test case too: #18926 (review)
-
MarcoFalke merged this on May 29, 2020
-
MarcoFalke closed this on May 29, 2020
-
sidhujag referenced this in commit 2f57154071 on May 31, 2020
-
MarcoFalke referenced this in commit 62d137ac3b on Jul 30, 2020
-
Fabcien referenced this in commit d0e6cf7186 on Dec 10, 2020
-
DrahtBot locked this on Feb 15, 2022