test: Usage of m_args vs. m_node.args/gArgs #25055

1. 
   dongcarl commented at 7:06 pm on May 2, 2022: member

   I was curious about this while perusing through setup_common.cpp. It seems to me that:

   1. m_node.args is an alias to gArgs
   2. m_args is its own thing
   3. Extra arguments supplied to *TestingSetup ctors are parsed into m_node.args/gArgs, m_args is left alone
   4. The only usage of m_args is in the chainstate loading sequence I added and in various tests where only its GetDataDirBase() member is called

   Therefore it seems that the current uses of m_args in the chainstate loading sequence in TestingSetup::TestingSetup is incorrect, and we should likely remove m_args to avoid further confusion since it’s not serving any useful purpose.

   Would love some context here!

2. 
   ryanofsky commented at 7:45 pm on May 2, 2022: member

   I think the goal is that code being tested by unit tests should be using settings from the BasicTestingSetup::m_args, not gArgs. m_node.args is a pointer that currently points to gArgs so current tests work, but will later point to m_args, so future tests will be self-contained.. Eventually gArgs should go away entirely, or if it is kept, maybe it will just be used to pass options to the test framework itself (logging options, random seeds, etc).

   Probably a good strategy for writing tests is to use m_args where possible, use m_node.args if that’s not possible, and avoid ever referencing gArgs directly. But if this is too confusing we could eliminiate m_args like you suggest. The last place this was discussed was #21244 (review) so that might provide more context.

3. MarcoFalke added the label Brainstorming on May 3, 2022
4. MarcoFalke added the label Tests on May 3, 2022
5. 
   MarcoFalke commented at 9:47 am on June 2, 2022: member

   Seems reasonable. Maybe add this as doxygen to m_args and close this issue?

Contributors
dongcarl ryanofsky MarcoFalke

Labels
Brainstorming Tests