test: Pass ArgsManager into getarg

glowang commented at 10:39 pm on May 9, 2020: contributor

Replaced 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

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 like

https://github.com/bitcoin/bitcoin/blob/db762529d6bffcc6ab0c1c64b9378925f2d75fa1/src/test/getarg_tests.cpp#L16-L23

https://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: member

Code 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: member

Concept 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 getting

cannot 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 not gArgs but line 214 still says gArgs.LogArgs() not argsman.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() to Loargsman(), 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 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: contributor

This 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: member

Almost-ACK

Create a local class inherited from BasicTestingSetup with a localized args manager

and put it into the getarg_tests namespace

357f02bf29

scripted-diff: replace gArgs with argsman

-BEGIN VERIFY SCRIPT-
sed -i 's/\<gArgs\>/m_args/g' src/test/getarg_tests.cpp
-END VERIFY SCRIPT-

f871f15c9d

glowang force-pushed on May 28, 2020

MarcoFalke commented at 1:39 pm on May 28, 2020: member

ACK 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: member

If 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

test: Pass ArgsManager into getarg_tests #18926