Do not use LocalTestingSetup in getarg_tests test file. #24375

1. 
   ryanofsky commented at 0:47 am on February 18, 2022: member

   Avoid using a test fixture in getarg_tests for better readability. Change was implemented by kiminuo and posted #24306 (comment)
2. DrahtBot added the label Tests on Feb 18, 2022
3. Do not use `LocalTestingSetup` in getarg_tests test file. 5d7f22595f
4. in src/test/getarg_tests.cpp:304 in 55059269fe outdated

   459-    SetupArgs({okaylog_bool, okaylog_negbool, okaylog, dontlog});
   460-    ResetArgs("-okaylog-bool -nookaylog-negbool -okaylog=public -dontlog=private");
   461+    SetupArgs(local_args, {okaylog_bool, okaylog_negbool, okaylog, dontlog});
   462+    ResetArgs(local_args, "-okaylog-bool -nookaylog-negbool -okaylog=public -dontlog=private");
   463+
   464+    LogInstance().StartLogging();
   

   ---

   ---

   

   MarcoFalke commented at 6:37 am on February 18, 2022:

   CI fails sequentially. Maybe due to this?

   ---

   

   kiminuo commented at 9:11 am on February 18, 2022:

   This line turns off the logger buffering feature: https://github.com/ryanofsky/bitcoin/blob/55059269fe7379912b315a80f3a1a17b33a3e23d/src/logging.cpp#L68

   so that https://github.com/ryanofsky/bitcoin/blob/55059269fe7379912b315a80f3a1a17b33a3e23d/src/logging.cpp#L267 is false and the logger callbacks are called

   https://github.com/ryanofsky/bitcoin/blob/55059269fe7379912b315a80f3a1a17b33a3e23d/src/logging.cpp#L278-L280

   I have tried to run locally src/test/test_bitcoin --log_level=all --run_test=getarg_tests and it passes. However, src/test/test_bitcoin does not pass for me so you are right and it can be reasonably reproduced. Also as one may expect variable s is empty (https://github.com/ryanofsky/bitcoin/blob/55059269fe7379912b315a80f3a1a17b33a3e23d/src/test/getarg_tests.cpp#L307).

   ---

   

   kiminuo commented at 10:52 am on February 18, 2022:

   So the error is because of this line https://github.com/ryanofsky/bitcoin/blob/55059269fe7379912b315a80f3a1a17b33a3e23d/src/logging.cpp#L53 as m_print_to_file is true sometimes so the callback is not called.

   ---

   

   kiminuo commented at 12:09 pm on February 18, 2022:

   https://github.com/kiminuo/bitcoin/commit/1ca557e57d0bdfb424ce4bf8bd7a1dd76b58be35 might actually fix the issue but I’m not sure what you would say about the change. Anyway, I don’t really know about a different solution so either that or closing #24375.

   This uncovers that working with static variables is hard :(

   ---

   

   ryanofsky commented at 12:14 pm on February 18, 2022:

   re: #24375 (review)

   I wasn’t really sure how to reproduce this but now I tried switching to the BasicTestingSetup fixture to see if that fixes it

   ---

   

   kiminuo commented at 12:53 pm on February 18, 2022:

   I used src/test/test_bitcoin --run_test=getarg_tests,fs_tests to reproduce the issue.

   ---

   

   kiminuo commented at 12:59 pm on February 18, 2022:

   My understanding is that Logger instance is shared between tests. So then fs_tests and getarg_tests interfere with each other because the BasicTestingSetup fixture sets a file name, then getarg_tests/logargs and its LogInstance().StartLogging(); does not turn off buffering of log messages and then the callback in logargs is not actually called.

   ---

   

   kiminuo commented at 12:11 pm on February 28, 2022:

   @ryanofsky Do you feel like switching to BasicTestingSetup is a good enough result for now or do you feel like it would be actually fruitful to remove the dependency on BasicTestingSetup?

   I think it is reasonable to reserve the later for a future PR as it seems it requires more changes.

   ---

   

   ryanofsky commented at 6:29 pm on February 28, 2022:

   @ryanofsky Do you feel like switching to BasicTestingSetup is a good enough result for now or do you feel like it would be actually fruitful to remove the dependency on BasicTestingSetup?

   I don’t think there’s a problem with depending on BasicTestingSetup here, but maybe I am coming at this from different perspective. I think global variables are almost always a problem, but test fixtures are only sometimes a problem (https://github.com/bitcoin/bitcoin/pull/22155#discussion_r673051693 was a place where I previously objected to using test fixtures unnecessarily).

5. 
   ryanofsky commented at 12:17 pm on February 18, 2022: member

   Updated 55059269fe7379912b315a80f3a1a17b33a3e23d -> 5d7f22595ff2de9b9883e468e3ce7182fc3f183b (pr/fixt.1 -> pr/fixt.2, compare) to try to fix https://cirrus-ci.com/task/6481959261044736 and https://cirrus-ci.com/task/6200484284334080.
6. ryanofsky force-pushed on Feb 18, 2022
7. 
   DrahtBot commented at 1:20 am on February 19, 2022: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #24306 (util: Make ArgsManager::GetPathArg more widely usable by ryanofsky)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

8. 
   kiminuo commented at 6:49 pm on February 28, 2022: contributor

   ACK 5d7f22595ff2de9b9883e468e3ce7182fc3f183b
9. MarcoFalke merged this on Mar 2, 2022
10. MarcoFalke closed this on Mar 2, 2022

    ---

11. sidhujag referenced this in commit 6dabdd11c8 on Mar 2, 2022
12. DrahtBot locked this on Mar 2, 2023

Contributors
ryanofsky MarcoFalke kiminuo DrahtBot

Labels
Tests