Do not use LocalTestingSetup in getarg_tests test file.
#24375
Conversation
src/test/getarg_tests.cpp
Outdated
| SetupArgs(local_args, {okaylog_bool, okaylog_negbool, okaylog, dontlog}); | ||
| ResetArgs(local_args, "-okaylog-bool -nookaylog-negbool -okaylog=public -dontlog=private"); | ||
|
|
||
| LogInstance().StartLogging(); |
There was a problem hiding this comment.
CI fails sequentially. Maybe due to this?
There was a problem hiding this comment.
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
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
kiminuo@1ca557e 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 :(
There was a problem hiding this comment.
re: #24375 (comment)
I wasn't really sure how to reproduce this but now I tried switching to the BasicTestingSetup fixture to see if that fixes it
There was a problem hiding this comment.
I used src/test/test_bitcoin --run_test=getarg_tests,fs_tests to reproduce the issue.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@ryanofsky Do you feel like switching to
BasicTestingSetupis a good enough result for now or do you feel like it would be actually fruitful to remove the dependency onBasicTestingSetup?
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 (#22155 (comment) was a place where I previously objected to using test fixtures unnecessarily).
There was a problem hiding this comment.
Updated 5505926 -> 5d7f225 (pr/fixt.1 -> pr/fixt.2, compare) to try to fix https://cirrus-ci.com/task/6481959261044736 and https://cirrus-ci.com/task/6200484284334080.
src/test/getarg_tests.cpp
Outdated
| SetupArgs(local_args, {okaylog_bool, okaylog_negbool, okaylog, dontlog}); | ||
| ResetArgs(local_args, "-okaylog-bool -nookaylog-negbool -okaylog=public -dontlog=private"); | ||
|
|
||
| LogInstance().StartLogging(); |
There was a problem hiding this comment.
re: #24375 (comment)
I wasn't really sure how to reproduce this but now I tried switching to the BasicTestingSetup fixture to see if that fixes it
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
ACK 5d7f225 |
…est file. 5d7f225 Do not use `LocalTestingSetup` in getarg_tests test file. (Kiminuo) Pull request description: Avoid using a test fixture in getarg_tests for better readability. Change was implemented by _kiminuo_ and posted bitcoin#24306 (comment) ACKs for top commit: kiminuo: ACK 5d7f225 Tree-SHA512: 0fd98622010e6923e91c66447a1d0861bf344a65d86a313dff7d428c089b1740a25f699327f6ed4c163255f270bcbd4f7be962bb551862214f9b9e395d40df04
Avoid using a test fixture in getarg_tests for better readability. Change was implemented by kiminuo and posted #24306 (comment)