Khalil/1895 Add config package unit test #4523
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4523 +/- ##
==========================================
+ Coverage 54.09% 57.74% +3.65%
==========================================
Files 563 511 -52
Lines 56220 47124 -9096
==========================================
- Hits 30414 27214 -3200
+ Misses 23459 18072 -5387
+ Partials 2347 1838 -509
Flags with carried forward coverage won't be shown. Click here to find out more.
|
config/config.go
Outdated
| func init() { | ||
| initialize() | ||
| } |
There was a problem hiding this comment.
nit: can you put this at the top of the file? It's easier to find that way.
config/config_test.go
Outdated
| func defaultConfig(t *testing.T) *FlowConfig { | ||
| initialize() | ||
| c, err := DefaultConfig() | ||
| require.NoError(t, err) | ||
| require.Equalf(t, "./default-config.yml", c.ConfigFile, "expected default config file to be used") | ||
| require.NoErrorf(t, c.Validate(), "unexpected error encountered validating default config") | ||
| return c | ||
| } |
There was a problem hiding this comment.
Suggestion: For enhanced reusability and encapsulation, it would be prudent to relocate this to the unittest package as an exportable test utility function. Additionally, please substitute all instances of config.DefaultConfig() within the test with unittest.DefaultConfig(t). Thank you!
config/README.md
Outdated
| The entire default config can be overridden using the `--config-file` CLI flag. When set the config package will attempt to parse the specified config file and override all the values | ||
| defined. A single default value can be overridden by setting the CLI flag for that specific config. For example `--network-connection-pruning=false` will override the default network connection pruning | ||
| config to false. | ||
|
|
There was a problem hiding this comment.
Based on this documentation, a comprehensive example might resemble the following command. If this aligns with the intended usage, please incorporate it into the readme. Alternatively, if there are any deviations, kindly add a complete, workable example. Users frequently find it more straightforward to emulate a practical example than to extract one from the documentation.
<rest of CLI command> --config-file=config/config.yml --network-connection-pruning=false
config/README.md
Outdated
| ### Adding a new config value | ||
| Adding a new config to the FlowConfig can be done in a few easy steps. | ||
|
|
||
| 1. Create a new subpackage for the config value. This package will define the configuration structs and CLI flags for overriding. |
There was a problem hiding this comment.
Please add the recommended package where the user can define this subpackage to. For example, are we encouraging the users to add the sub-packages to /config package?
| 2. `mapstructure` - mapstructure tag is used for unmarshalling and must match the CLI flag name defined in step or else the field will not be set when the config is unmarshalled. | ||
| ```go | ||
| type MyComponentConfig struct { | ||
| AppWorkers int `validate:"gt=0" mapstructure:"app-workers"` |
There was a problem hiding this comment.
In the above example, it is important to elucidate how the configuration is processed, for example, focusing on the AppWorkers field. The AppWorkers field is populated with the value corresponding to app-workers from the configuration file. It's essential to note that there is a validation check in place to ensure that this value is greater than zero. This validation is critical because a non-positive value for the number of workers would not make sense and could cause runtime issues. In case the validation fails, meaning if the value is set to zero or a negative number, the code is expected to handle this scenario appropriately. Typically, this would involve throwing an error or an exception to indicate that an invalid configuration value was provided. Please document what would be such expected behavior.
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>
Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>
…w/flow-go into khalil/1895-config-system-unittest
…onfig-system-unittest
This PR adds some unit test coverage to the new config package. It also adds a README file with details about the package and instructions on how to add a new configuration value.