rubyconfig / config

Easiest way to add multi-environment yaml settings to Rails, Sinatra, Padrino and other Ruby projects.
Other
2.1k stars 230 forks source link

Run tests in random order #298

Closed cjlarose closed 3 years ago

cjlarose commented 3 years ago

Fixes #198 Fixes #231

pkuczynski commented 3 years ago

Why do we need to set fail_on_missing = false?

cjlarose commented 3 years ago

Why do we need to set fail_on_missing = false?

There a a good number of tests that expect fail_on_missing to be false, which is pretty reasonable considering that's the default setting. Here's two examples from the env suite:

https://github.com/rubyconfig/config/blob/c59d105a870032c8ed1796d6c3f5d8e153ef991c/spec/config_env_spec.rb#L109-L119

If fail_on_missing is true, these examples encounter a KeyError instead of just getting nil back when they access a non-existent key.

An alternative fix I considered is to just add an after :each for the examples that modify the setting, so they don't end up affecting other tests.

git apply <<'PATCH'
diff --git a/spec/options_spec.rb b/spec/options_spec.rb
index 8f49a8d..9287c77 100644
--- a/spec/options_spec.rb
+++ b/spec/options_spec.rb
@@ -132,6 +132,7 @@ describe Config::Options do
   context 'when fail_on_missing option' do
     context 'is set to true' do
       before { Config.setup { |cfg| cfg.fail_on_missing = true } }
+      after { Config.setup { |cfg| cfg.fail_on_missing = false } }

       it 'should raise an error when accessing a missing key' do
         config = Config.load_files("#{fixture_path}/empty1.yml")
PATCH

In general though, I think this approach is prone to error since it's easy to forget to add something like this when someone introduces a new test that modifies config attributes. The cost is relatively small to just Config.reset in a before :each and seems much less prone to error.

cjlarose commented 3 years ago

@pkuczynski LMK if this looks good!