Settings, defaults, and tests

First I’d like to air an assumption I got by osmosis:

Code can rely on settings to exist and have a sensible default.

If settings.yml has :foo: :bar: true, code can do if without worrying that foo or bar can be entirely missing (which would return nil or blow up respectively).

When naming, we care about how it reads, e.g. if do_it is better than the double negative unless skip_it. We don’t care which value is “typical” or backward-compatible.


Implications for tests

To what degree tests can/should rely on settings.yml vs providing their own settings?

stub_settings vs stub_settings_merge

stub_settings replaces the top-level Settings completely with what you provide. You can’t possibly provide everything, so most settings will be missing. Code that doesn’t expect this can misbehave in unexpected ways, so AFAICT it’s never safe to use (except in tests of Settings machinery itself).

=> stub_settings_merge deep-merges what you provide on top of settings.yml, and is what you want in tests.

Config injection

[This is prompted by #14662 discussion.
The code under test nicely takes a portion of settings as an argument, allowing injecting OpenStruct or similar in tests :+1:. @enoodle caught me injecting the actual live Settings :-1: ;-).]

Again, I’m worried that if I write tests injecting options :foo, :bar and later a :baz setting appears, in some cases options.baz being nil may silently do silly things (like exit early), and test might pass despite no longer testing any actual behavior.

=> The solution I came up with is injecting a Struct instead of OpenStruct or Config::Options.
This way if code started accessing options.baz the test would blow up, and we’ll remember to add the option in test.
What do you think?

I also think, we should merge settings as the default, because new options can make the code behave different. The specs should tweak settings from default to test different behaviour.

See for making the change.

I’m not sure if a Struct is really necessary.

Fryguy also suggested upstream an option to raise on non-existant keys: (no response so far)