Page MenuHomePhabricator
Differential D10828

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

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

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

thonkle created this revision.Jan 14 2022, 00:51
Owners added a reviewer: Restricted Owners Package.Jan 14 2022, 00:51
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 14 2022, 00:51
thonkle requested review of this revision.Jan 14 2022, 00:51
Harbormaster completed remote builds in B17908: Diff 31806.Jan 14 2022, 01:07
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
thonkle added a comment.Jan 14 2022, 16:14
In D10828#242951, @Fabien wrote:

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.

thonkle updated this revision to Diff 31815.Jan 14 2022, 16:16

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

Harbormaster completed remote builds in B17916: Diff 31815.Jan 14 2022, 16:31
Fabien requested changes to this revision.Jan 16 2022, 14:37
Fabien added inline comments.
src/seeder/options.h
30 ↗(On Diff #31815)

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
thonkle updated this revision to Diff 31832.Jan 18 2022, 15:13

Make argsManager a pointer to gArgs

Harbormaster completed remote builds in B17930: Diff 31832.Jan 18 2022, 15:24
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
thonkle updated this revision to Diff 31845.Jan 19 2022, 15:34

Use constants in tests

Harbormaster completed remote builds in B17940: Diff 31845.Jan 19 2022, 15:52
Fabien accepted this revision.Jan 19 2022, 16:23
This revision is now accepted and ready to land.Jan 19 2022, 16:23
Closed by commit rABC35d09a72e046: Move SetupSeederArgs out of parsing code to make it easier to test (authored by thonkle). · Explain WhyJan 19 2022, 18:19
This revision was automatically updated to reflect the committed changes.
thonkle added a commit: rABC35d09a72e046: Move SetupSeederArgs out of parsing code to make it easier to test.

Revision Contents
Changeset List

PathSize
src/
seeder/
3 lines
18 lines
125 lines
test/
22 lines

Diff 31854

View Options

src/seeder/main.cpp

Loading...
View Options

src/seeder/options.h

Loading...
View Options

src/seeder/options.cpp

Loading...
View Options

src/seeder/test/options_tests.cpp

Loading...