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
  1. 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
  2. fanquake added the label Tests on May 9, 2020
  3. fanquake requested review from ryanofsky on May 10, 2020
  4. glowang renamed this:
    Pass ArgsManager into functions that register args with AddArg
    Pass ArgsManager into getarg_tests
    on May 10, 2020
  5. glowang marked this as ready for review on May 10, 2020
  6. glowang force-pushed on May 12, 2020
  7. glowang force-pushed on May 12, 2020
  8. 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.
  9. 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!
  10. ryanofsky approved
  11. 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.
  12. glowang force-pushed on May 13, 2020
  13. 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/).
    
  14. 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
  15. MarcoFalke referenced this in commit fa1552b6ef on May 14, 2020
  16. glowang force-pushed on May 15, 2020
  17. glowang force-pushed on May 16, 2020
  18. 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

  19. glowang force-pushed on May 19, 2020
  20. 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.
  21. 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 🙏
  22. glowang force-pushed on May 23, 2020
  23. glowang force-pushed on May 23, 2020
  24. glowang force-pushed on May 23, 2020
  25. glowang requested review from MarcoFalke on May 23, 2020
  26. glowang requested review from ryanofsky on May 23, 2020
  27. glowang requested review from dongcarl on May 23, 2020
  28. glowang commented at 6:11 pm on May 23, 2020: contributor
    This is ready for re-review.
  29. MarcoFalke renamed this:
    Pass ArgsManager into getarg_tests
    test: Pass ArgsManager into getarg_tests
    on May 23, 2020
  30. 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)

  31. 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

  32. MarcoFalke commented at 8:50 pm on May 23, 2020: member
    Almost-ACK
  33. Create a local class inherited from BasicTestingSetup with a localized args manager
    and put it into the getarg_tests namespace
    357f02bf29
  34. 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
  35. glowang force-pushed on May 28, 2020
  36. MarcoFalke commented at 1:39 pm on May 28, 2020: member
    ACK f871f15c9d760b56a6a3b78b3941d3983d60810c
  37. ryanofsky approved
  38. 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

  39. 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)
  40. MarcoFalke merged this on May 29, 2020
  41. MarcoFalke closed this on May 29, 2020

  42. sidhujag referenced this in commit 2f57154071 on May 31, 2020
  43. MarcoFalke referenced this in commit 62d137ac3b on Jul 30, 2020
  44. Fabcien referenced this in commit d0e6cf7186 on Dec 10, 2020
  45. DrahtBot locked this on Feb 15, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-21 03:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me