Skip to content

Conversation

@cjlarose
Copy link
Member

@cjlarose cjlarose commented Dec 12, 2020

Fixes #198
Fixes #231

This fixes the `const_name` tests in particular to be order-independent,
but also just makes it more difficult in general to introduce new tests
that are order-dependent.
This'll make it easier for tests to get a clean set of options,
especially if they run after tests that modify `fail_on_missing`.
This makes many tests in this suite order-independent. Without it,
several tests fail if executed after a test that sets `fail_on_missing`
to `true`.
@pkuczynski
Copy link
Member

Why do we need to set fail_on_missing = false?

@cjlarose
Copy link
Member Author

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:

it 'should not load variables from the default prefix' do
ENV['Settings.new_var'] = 'value'
expect(config.new_var).to eq(nil)
end
it 'should skip ENV variable when partial prefix match' do
ENV['MyConfigs.new_var'] = 'value'
expect(config.new_var).to eq(nil)
end

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
Copy link
Member Author

@pkuczynski LMK if this looks good!

@pkuczynski pkuczynski merged commit c53194d into rubyconfig:master Dec 20, 2020
@pkuczynski pkuczynski changed the title Run examples in random order Run tests in random order Dec 20, 2020
ippachi pushed a commit to ippachi/config-1 that referenced this pull request Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Specs rely on test order

2 participants