Page MenuHomePhabricator

Move SetupSeederArgs out of parsing code to make it easier to test
ClosedPublic

Authored by thonkle on Jan 14 2022, 00:51.

Details

Reviewers
Fabien
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC35d09a72e046: Move SetupSeederArgs out of parsing code to make it easier to test
Summary

Trying to add more test cases to options_tests.cpp results in errors
due to the way the seeder sets up the ArgsManager on each call to ParseCommandLine.
It doesn't make sense to setup args while parsing anyway, so moving it feels right.

The patch includes adding a test case to demonstrate that the change works as intended.

Test Plan
ninja check-seeder bitcoin-seeder

Diff Detail

Event Timeline

Owners added a reviewer: Restricted Owners Package.Jan 14 2022, 00:51
thonkle requested review of this revision.Jan 14 2022, 00:51
Fabien requested changes to this revision.Jan 14 2022, 08:37
Fabien added a subscriber: Fabien.

What do you think of passing an ArgsManager to the CDnsSeedOpts instead ? So in main you can pass gArgs, and in tests whatever local dummy ArgsManager you want. This makes it possible to keep the method as is with the added benefit of making it simple to test by removing the dependency to the global variable gArgs.

This revision now requires changes to proceed.Jan 14 2022, 08:37

What do you think of passing an ArgsManager to the CDnsSeedOpts instead ? So in main you can pass gArgs, and in tests whatever local dummy ArgsManager you want. This makes it possible to keep the method as is with the added benefit of making it simple to test by removing the dependency to the global variable gArgs.

I considered this but dismissed it because I had convinced myself SeederSetupArgs didn't belong as part of CDnsSeedOpts. However implementing your suggestion actually revealed a bug where its possible to pass a different ArgsManager to SeederSetupArgs and that will not match the hardcoded gArgs in ParseCommandLine.
This is worth fixing, and therefore my original assumption was likely incorrect.

Refactor gArgs out of CDnsSeedOpts and pass in ArgsManager for finer control while testing

Fabien requested changes to this revision.Jan 16 2022, 14:37
Fabien added inline comments.
src/seeder/options.h
30

It's a bad idea to store a ref here. You can store by value and use a move initializer in the constructor

This revision now requires changes to proceed.Jan 16 2022, 14:37

Make argsManager a pointer to gArgs

Fabien requested changes to this revision.Jan 18 2022, 16:40
Fabien added inline comments.
src/seeder/options.h
28 ↗(On Diff #31832)

Not for this diff, but if everything is public better make it a struct

src/seeder/test/options_tests.cpp
23 ↗(On Diff #31832)

You recently defined constants for this values

This revision now requires changes to proceed.Jan 18 2022, 16:40
This revision is now accepted and ready to land.Jan 19 2022, 16:23