Fix:Options#using_default? on env var

[marcotc]

Today, Options#using_default? always returns false if the first interaction with that option is a call to Options#using_default?.

This is an issue because the underlying Option did not have the chance to try to resolve its environment variable. If it did, it might actually not be using the default: value, but the env: value instead.

This PR ensures that Options#using_default? always returns a reliable result under every condition.

[marcotc]

This is a small refactor to this file, because add_option(name) unless options.key?(name) was too common and not trivial to understand.

[GustavoCaso]

Suggested change

	describe '#using_default?' do
	subject(:using_default?) { options_object.using_default?(name) }
	let(:name) { :foo }
	context 'when the option is defined' do
	before { options_class.send(:option, name, meta) }
	let(:meta) { {} }
	context 'and a value is set' do
	let(:value) { double('value') }
	before { options_object.set_option(name, value) }
	it { is_expected.to be(false) }
	end
	context 'and a value is not set' do
	context 'and no value is configured' do
	it { is_expected.to be(true) }
	end
	context 'and a default is configured' do
	let(:meta) { super().merge(default: 'anything') }
	it { is_expected.to be(true) }
	end
	context 'and an environment variable is configured' do
	let(:meta) { super().merge(env: 'TEST_ENV_VAR') }
	around do |example|
	ClimateControl.modify('TEST_ENV_VAR' => 'anything') { example.run }
	end
	it { is_expected.to be(false) }
	end
	end
	end
	context 'when the option is not defined' do
	it { expect { using_default? }.to raise_error(described_class::InvalidOptionError) }
	end
	end
	describe '#using_default?' do
	subject(:using_default?) { options_object.using_default?(name) }
	let(:name) { :foo }
	context 'when the option is defined' do
	before { options_class.send(:option, name, meta) }
	let(:meta) { {} }
	context 'and a value is set' do
	before { options_object.set_option(name, 'something') }
	it { is_expected.to be(false) }
	end
	context 'and a value is not set' do
	context 'and no default value is configured' do
	it { is_expected.to be(true) }
	end
	context 'and a default value is configured' do
	let(:meta) { { default: 'anything' } }
	it { is_expected.to be(true) }
	context 'and an environment variable is configured' do
	let(:meta) { { default: 'anything', env: 'TEST_ENV_VAR' } }
	context 'and an environmet variable is set' do
	around do |example|
	ClimateControl.modify('TEST_ENV_VAR' => 'anything') { example.run }
	end
	it { is_expected.to be(false) }
	end
	context 'and an environment variable is not set' do
	it { is_expected.to be(true) }
	end
	end
	end
	context 'an environment variable is configured' do
	let(:meta) { { env: 'TEST_ENV_VAR' } }
	context 'and an environmet variable is set' do
	around do |example|
	ClimateControl.modify('TEST_ENV_VAR' => 'anything') { example.run }
	end
	it { is_expected.to be(false) }
	end
	context 'and an environment variable is not set' do
	it { is_expected.to be(true) }
	end
	end
	end
	end

The suggestion includes:

• Remove calls to super inside let
• Changed some test description to be more specific
• Add a case for when the env option is set but the ENV variable is not configured
• Add a context for env is defined without default value

[GustavoCaso]

Also, I wrote some specs on

dd-trace-rb/spec/datadog/core/configuration/base_spec.rb

Lines 145 to 218 in 37d556d

	describe '#using_default?' do
	let(:configuration) do
	Class.new do
	include Datadog::Core::Configuration::Base
	settings :fake_test do
	option :enabled, default: true
	option :without_default
	end
	end.new
	end
	context 'with default option' do
	context 'when not set or accessed' do
	it 'returns true' do
	expect(configuration.fake_test.using_default?(:enabled)).to be(true)
	end
	end
	context 'when not set but accessed' do
	it 'returns true' do
	configuration.fake_test.enabled
	expect(configuration.fake_test.using_default?(:enabled)).to be(true)
	end
	end
	context 'when set' do
	it 'returns false' do
	configuration.fake_test.enabled = false
	expect(configuration.fake_test.using_default?(:enabled)).to be(false)
	end
	end
	context 'when set and reset' do
	it 'returns false' do
	configuration.fake_test.enabled = false
	expect(configuration.fake_test.using_default?(:enabled)).to be(false)
	configuration.fake_test.reset!
	expect(configuration.fake_test.using_default?(:enabled)).to be(true)
	end
	end
	end
	context 'without default option' do
	context 'when not set or accessed' do
	it 'returns true' do
	expect(configuration.fake_test.using_default?(:without_default)).to be(true)
	end
	end
	context 'when not set but accessed' do
	it 'returns true' do
	configuration.fake_test.without_default
	expect(configuration.fake_test.using_default?(:without_default)).to be(true)
	end
	end
	context 'when set' do
	it 'returns false' do
	configuration.fake_test.without_default = false
	expect(configuration.fake_test.using_default?(:without_default)).to be(false)
	end
	end
	context 'when set and reset' do
	it 'returns false' do
	configuration.fake_test.without_default = false
	expect(configuration.fake_test.using_default?(:without_default)).to be(false)
	configuration.fake_test.reset!
	expect(configuration.fake_test.using_default?(:without_default)).to be(true)
	end
	end
	end
	end

What do you think about removing those? I feel they test the same thing (without the env part) I think your specs are better suited to test the feature fully.

[marcotc]

Thank you for the updates, applied!

What do you think about removing those?

I think it's fine to remove those, as they seem a bit redundant like you said. I went ahead and did that.

[GustavoCaso]

Thank you so much for fixing this @marcotc 🎉

I left a few comments regarding the spec 😄

[codecov-commenter]

Codecov Report

Merging #2994 (97025c6) into master (ff0667a) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2994      +/-   ##
==========================================
- Coverage   98.09%   98.09%   -0.01%     
==========================================
  Files        1305     1305              
  Lines       72724    72709      -15     
  Branches     3360     3357       -3     
==========================================
- Hits        71336    71321      -15     
  Misses       1388     1388              

Files Changed	Coverage Δ
spec/datadog/core/configuration/base_spec.rb	100.00% <ø> (ø)
lib/datadog/core/configuration/options.rb	100.00% <100.00%> (ø)
spec/datadog/core/configuration/options_spec.rb	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more