fix all sorts of issues with random spec order

SiteSetting no longer persisted across tests

lib/site_setting_extension.rb

@@ -1,7 +1,18 @@
 require_dependency 'enum'
+require_dependency 'site_settings/db_provider'
 
 module SiteSettingExtension
 
+  # part 1 of refactor, centralizing the dependency here
+  def provider=(val)
+    @provider = val
+    refresh!
+  end
+
+  def provider
+    @provider ||= SiteSettings::DbProvider.new(SiteSetting)
+  end
+
   def types
     @types ||= Enum.new(:string, :time, :fixnum, :float, :bool, :null, :enum)
   end
@@ -12,7 +23,7 @@ module SiteSettingExtension
 
   def current
     @@containers ||= {}
-    @@containers[RailsMultisite::ConnectionManagement.current_db] ||= {}
+    @@containers[provider.current_site] ||= {}
   end
 
   def defaults
@@ -27,7 +38,10 @@ module SiteSettingExtension
     mutex.synchronize do
       self.defaults[name] = default
       current_value = current.has_key?(name) ? current[name] : default
-      enums[name] = opts[:enum] if opts[:enum]
+      if opts[:enum]
+        enum = opts[:enum]
+        enums[name] = enum.is_a?(String) ? enum.constantize : enum
+      end
       setup_methods(name, current_value)
     end
   end
@@ -74,29 +88,21 @@ module SiteSettingExtension
     I18n.t("site_settings.#{setting}")
   end
 
-  # table is not in the db yet, initial migration, etc
-  def table_exists?
-    @table_exists = ActiveRecord::Base.connection.table_exists? 'site_settings' if @table_exists == nil
-    @table_exists
-  end
-
   def self.client_settings_cache_key
     "client_settings_json"
   end
 
   # refresh all the site settings
   def refresh!
-    return unless table_exists?
     mutex.synchronize do
       ensure_listen_for_changes
       old = current
 
-      all_settings = SiteSetting.select([:name,:value,:data_type])
+      all_settings = provider.all
       new_hash =  Hash[*(all_settings.map{|s| [s.name.intern, convert(s.value,s.data_type)]}.to_a.flatten)]
 
       # add defaults
       new_hash = defaults.merge(new_hash)
-
       changes,deletions = diff_hash(new_hash, old)
 
       if deletions.length > 0 || changes.length > 0
@@ -129,7 +135,7 @@ module SiteSettingExtension
       begin
         @last_message_processed = message.global_id
         MessageBus.on_connect.call(message.site_id)
-        SiteSetting.refresh!
+        refresh!
       ensure
         MessageBus.on_disconnect.call(message.site_id)
       end
@@ -147,14 +153,11 @@ module SiteSettingExtension
   end
 
   def remove_override!(name)
-    return unless table_exists?
-    SiteSetting.where(name: name).destroy_all
+    provider.destroy(name)
+    current[name] = defaults[name]
   end
 
   def add_override!(name,val)
-    return unless table_exists?
-
-    setting = SiteSetting.where(name: name).first
     type = get_data_type(name, defaults[name])
 
     if type == types[:bool] && val != true && val != false
@@ -173,18 +176,10 @@ module SiteSettingExtension
       raise Discourse::InvalidParameters.new(:value) unless enum_class(name).valid_value?(val)
     end
 
-    if setting
-      setting.value = val
-      setting.data_type = type
-      setting.save
-    else
-      SiteSetting.create!(name: name, value: val, data_type: type)
-    end
-
+    provider.save(name, val, type)
     @last_message_sent = MessageBus.publish('/site_settings', {process: process_id})
   end
 
-
   protected
 
   def diff_hash(new_hash, old)
@@ -225,7 +220,7 @@ module SiteSettingExtension
     when types[:string], types[:enum]
       value
     when types[:bool]
-      value == "t"
+      value == true || value == "t" || value == "true"
     when types[:null]
       nil
     end
@@ -235,16 +230,12 @@ module SiteSettingExtension
   def setup_methods(name, current_value)
 
     # trivial multi db support, we can optimize this later
-    db = RailsMultisite::ConnectionManagement.current_db
-
-    @@containers ||= {}
-    @@containers[db] ||= {}
-    @@containers[db][name] = current_value
+    current[name] = current_value
 
     setter = ("#{name}=").sub("?","")
 
     eval "define_singleton_method :#{name} do
-      c = @@containers[RailsMultisite::ConnectionManagement.current_db]
+      c = @@containers[provider.current_site]
       c = c[name] if c
       c
     end
@@ -265,7 +256,6 @@ module SiteSettingExtension
   end
 
   def enum_class(name)
-    enums[name] = enums[name].constantize unless enums[name].is_a?(Class)
     enums[name]
   end
 

lib/site_settings/db_provider.rb

@@ -0,0 +1,65 @@
+module SiteSettings; end
+
+class SiteSettings::DbProvider
+
+  def initialize(model)
+    @model = model
+  end
+
+  def all
+    return [] unless table_exists?
+
+    # note, not leaking out AR records, cause I want all editing to happen
+    # via this API
+    SqlBuilder.new("select name, data_type, value from #{@model.table_name}").map_exec(OpenStruct)
+  end
+
+  def find(name)
+    return nil unless table_exists?
+
+    # note, not leaking out AR records, cause I want all editing to happen
+    # via this API
+    SqlBuilder.new("select name, data_type, value from #{@model.table_name} where name = :name")
+      .map_exec(OpenStruct, name: name)
+      .first
+  end
+
+  def save(name, value, data_type)
+
+    return unless table_exists?
+
+    count = @model.update_all({
+      name: name,
+      value: value,
+      data_type: data_type,
+      updated_at: Time.now
+    }, {
+      name: name
+    })
+
+    if count == 0
+      @model.create!(name: name, value: value, data_type: data_type)
+    end
+
+    true
+  end
+
+  def destroy(name)
+    return unless table_exists?
+
+    @model.where(name: name).destroy_all
+  end
+
+  def current_site
+    RailsMultisite::ConnectionManagement.current_db
+  end
+
+  protected
+
+  # table is not in the db yet, initial migration, etc
+  def table_exists?
+    @table_exists = ActiveRecord::Base.connection.table_exists? @model.table_name if @table_exists == nil
+    @table_exists
+  end
+
+end

lib/site_settings/local_process_provider.rb

@@ -0,0 +1,31 @@
+module SiteSettings; end
+
+class SiteSettings::LocalProcessProvider
+
+  Setting = Struct.new(:name, :value, :data_type)
+
+  def initialize
+    @settings = {}
+  end
+
+  def all
+    @settings.values
+  end
+
+  def find(name)
+    @settings[name]
+  end
+
+  def save(name, value, data_type)
+    @settings[name] = Setting.new(name,value, data_type)
+  end
+
+  def destroy(name)
+    @settings.delete(name)
+  end
+
+  def current_site
+    "test"
+  end
+
+end

spec/components/site_setting_extenstion_spec.rb

@@ -0,0 +1,159 @@
+require 'spec_helper'
+require_dependency 'site_setting_extension'
+require_dependency 'site_settings/local_process_provider'
+
+describe SiteSettingExtension do
+
+  class FakeSettings
+    extend SiteSettingExtension
+    provider = SiteSettings::LocalProcessProvider
+  end
+
+  let :settings do
+    FakeSettings
+  end
+
+  describe "int setting" do
+    before do
+      settings.setting(:test_setting, 77)
+      settings.refresh!
+    end
+
+    it "should have a key in all_settings" do
+      settings.all_settings.detect {|s| s[:setting] == :test_setting }.should be_present
+    end
+
+    it "should have the correct desc" do
+      I18n.expects(:t).with("site_settings.test_setting").returns("test description")
+      settings.description(:test_setting).should == "test description"
+    end
+
+    it "should have the correct default" do
+      settings.test_setting.should == 77
+    end
+
+    context "when overidden" do
+      after :each do
+        settings.remove_override!(:test_setting)
+      end
+
+      it "should have the correct override" do
+        settings.test_setting = 100
+        settings.test_setting.should == 100
+      end
+
+      it "should coerce correct string to int" do
+        settings.test_setting = "101"
+        settings.test_setting.should.eql? 101
+      end
+
+      it "should coerce incorrect string to 0" do
+        settings.test_setting = "pie"
+        settings.test_setting.should.eql? 0
+      end
+
+			it "should not set default when reset" do
+        settings.test_setting = 100
+        settings.setting(:test_setting, 77)
+        settings.refresh!
+        settings.test_setting.should_not == 77
+      end
+    end
+  end
+
+  describe "remove_override" do
+    it "correctly nukes overrides" do
+      settings.setting(:test_override, "test")
+      settings.test_override = "bla"
+      settings.remove_override!(:test_override)
+      expect(settings.test_override).to eq("test")
+    end
+  end
+
+  describe "string setting" do
+    before do
+      settings.setting(:test_str, "str")
+      settings.refresh!
+    end
+
+    it "should have the correct default" do
+      settings.test_str.should == "str"
+    end
+
+    context "when overridden" do
+      after :each do
+        settings.remove_override!(:test_str)
+      end
+
+      it "should coerce int to string" do
+        settings.test_str = 100
+        settings.test_str.should.eql? "100"
+      end
+    end
+  end
+
+  describe "bool setting" do
+    before do
+      settings.setting(:test_hello?, false)
+      settings.refresh!
+    end
+
+    it "should have the correct default" do
+      settings.test_hello?.should == false
+    end
+
+    context "when overridden" do
+      after do
+        settings.remove_override!(:test_hello?)
+      end
+
+      it "should have the correct override" do
+        settings.test_hello = true
+        settings.test_hello?.should == true
+      end
+
+      it "should coerce true strings to true" do
+        settings.test_hello = "true"
+        settings.test_hello?.should.eql? true
+      end
+
+      it "should coerce all other strings to false" do
+        settings.test_hello = "f"
+        settings.test_hello?.should.eql? false
+      end
+
+			it "should not set default when reset" do
+        settings.test_hello = true
+        settings.setting(:test_hello?, false)
+        settings.refresh!
+        settings.test_hello?.should_not == false
+      end
+    end
+  end
+
+  describe 'enum setting' do
+    before do
+      @enum_class = Enum.new(:test)
+      settings.setting(:test_enum, 'en', enum: @enum_class)
+      settings.refresh!
+    end
+
+    it 'should have the correct default' do
+      expect(settings.test_enum).to eq('en')
+    end
+
+    context 'when overridden' do
+
+      it 'stores valid values' do
+        @enum_class.expects(:valid_value?).with('fr').returns(true)
+        settings.test_enum = 'fr'
+        expect(settings.test_enum).to eq('fr')
+      end
+
+      it 'rejects invalid values' do
+        @enum_class.expects(:valid_value?).with('gg').returns(false)
+        expect {settings.test_enum = 'gg' }.to raise_error(Discourse::InvalidParameters)
+      end
+    end
+  end
+end

spec/components/site_settings/db_provider_spec.rb

@@ -0,0 +1,51 @@
+require 'spec_helper'
+require_dependency 'site_settings/db_provider'
+
+describe SiteSettings::DbProvider do
+
+  def expect_same_setting(actual, expected)
+    expect(actual.name).to eq(expected.name)
+    expect(actual.value).to eq(expected.value)
+    expect(actual.data_type).to eq(expected.data_type)
+  end
+
+  let :provider do
+    SiteSettings::DbProvider.new(SiteSetting)
+  end
+
+  # integration test, requires db access
+  it "act correctly" do
+    setting = Struct.new(:name, :value, :data_type)
+
+    SiteSetting.destroy_all
+
+    expect(provider.all.length).to eq(0)
+    expect(provider.find("test")).to eq(nil)
+
+
+    provider.save("test", "one", 1)
+    found = provider.find("test")
+
+    expect_same_setting(found, setting.new("test", "one", 1))
+
+    provider.save("test", "two", 2)
+    found = provider.find("test")
+
+    expect_same_setting(found, setting.new("test", "two", 2))
+
+    provider.save("test2", "three", 3)
+
+    all = provider.all.sort{|a,b| a.name <=> b.name}
+
+    expect_same_setting(all[0], setting.new("test", "two", 2))
+    expect_same_setting(all[1], setting.new("test2", "three", 3))
+    expect(all.length).to eq(2)
+
+    provider.destroy("test")
+    expect(provider.all.length).to eq(1)
+  end
+
+  it "returns the correct site name" do
+    expect(provider.current_site).to eq(RailsMultisite::ConnectionManagement.current_db)
+  end
+end

spec/components/site_settings/local_process_provider_spec.rb

@@ -0,0 +1,70 @@
+require 'spec_helper'
+require_dependency 'site_settings/local_process_provider'
+
+describe SiteSettings::LocalProcessProvider do
+
+  def expect_same_setting(actual, expected)
+    expect(actual.name).to eq(expected.name)
+    expect(actual.value).to eq(expected.value)
+    expect(actual.data_type).to eq(expected.data_type)
+  end
+
+  let :provider do
+    SiteSettings::LocalProcessProvider.new
+  end
+
+
+  def setting(name,value,data_type)
+    OpenStruct.new.tap do |setting|
+      setting.name = name
+      setting.value = value
+      setting.data_type = data_type
+    end
+  end
+
+  describe "all" do
+    it "starts off empty" do
+      expect(provider.all).to eq([])
+    end
+
+    it "can allows additional settings" do
+      provider.save("test", "bla", 2)
+      expect_same_setting(provider.all[0], setting("test","bla",2))
+    end
+
+    it "does not leak new stuff into list" do
+      provider.save("test", "bla", 2)
+      provider.save("test", "bla1", 2)
+      expect_same_setting(provider.all[0], setting("test","bla1",2))
+      expect(provider.all.length).to eq(1)
+    end
+  end
+
+  describe "find" do
+    it "starts off empty" do
+      expect(provider.find("bla")).to eq(nil)
+    end
+
+    it "can find a new setting" do
+      provider.save("one","two",3)
+      expect_same_setting(provider.find("one"),setting("one", "two", 3))
+    end
+
+    it "can amend a setting" do
+      provider.save("one","three",4)
+      expect_same_setting(provider.find("one"),setting("one", "three", 4))
+    end
+  end
+
+  describe "destroy" do
+    it "can destroy a setting" do
+      provider.save("one","three",4)
+      provider.destroy("one")
+      expect(provider.find("one")).to eq(nil)
+    end
+  end
+
+  it "returns the correct site name" do
+    expect(provider.current_site).to eq("test")
+  end
+end

spec/models/site_setting_spec.rb

@@ -4,173 +4,29 @@ require_dependency 'site_setting_extension'
 
 describe SiteSetting do
 
-  describe "int setting" do
-    before :all do
-      SiteSetting.setting(:test_setting, 77)
-      SiteSetting.refresh!
-    end
-
-    it "should have a key in all_settings" do
-      SiteSetting.all_settings.detect {|s| s[:setting] == :test_setting }.should be_present
-    end
-
-    it "should have the correct desc" do
-      I18n.expects(:t).with("site_settings.test_setting").returns("test description")
-      SiteSetting.description(:test_setting).should == "test description"
-    end
-
-    it "should have the correct default" do
-      SiteSetting.test_setting.should == 77
-    end
-
-    context "when overidden" do
-      after :each do
-        SiteSetting.remove_override!(:test_setting)
-      end
-
-      it "should have the correct override" do
-        SiteSetting.test_setting = 100
-        SiteSetting.test_setting.should == 100
-      end
-
-      it "should coerce correct string to int" do
-        SiteSetting.test_setting = "101"
-        SiteSetting.test_setting.should.eql? 101
-      end
-
-      #POSSIBLE BUG
-      it "should coerce incorrect string to 0" do
-        SiteSetting.test_setting = "pie"
-        SiteSetting.test_setting.should.eql? 0
-      end
-
-      #POSSIBLE BUG
-			it "should not set default when reset" do
-        SiteSetting.test_setting = 100
-        SiteSetting.setting(:test_setting, 77)
-        SiteSetting.refresh!
-        SiteSetting.test_setting.should_not == 77
-      end
-    end
-  end
-
-  describe "string setting" do
-    before :all do
-      SiteSetting.setting(:test_str, "str")
-      SiteSetting.refresh!
-    end
-
-    it "should have the correct default" do
-      SiteSetting.test_str.should == "str"
-    end
-
-    context "when overridden" do
-      after :each do
-        SiteSetting.remove_override!(:test_str)
-      end
-
-      it "should coerce int to string" do
-        SiteSetting.test_str = 100
-        SiteSetting.test_str.should.eql? "100"
-      end
-    end
-  end
-
-  describe "bool setting" do
-    before :all do
-      SiteSetting.setting(:test_hello?, false)
-      SiteSetting.refresh!
-    end
-
-    it "should have the correct default" do
-      SiteSetting.test_hello?.should == false
-    end
-
-    context "when overridden" do
-      after :each do
-        SiteSetting.remove_override!(:test_hello?)
-      end
-
-      it "should have the correct override" do
-        SiteSetting.test_hello = true
-        SiteSetting.test_hello?.should == true
-      end
-
-      it "should coerce true strings to true" do
-        SiteSetting.test_hello = "true"
-        SiteSetting.test_hello?.should.eql? true
-      end
-
-      it "should coerce all other strings to false" do
-        SiteSetting.test_hello = "f"
-        SiteSetting.test_hello?.should.eql? false
-      end
-
-      #POSSIBLE BUG
-			it "should not set default when reset" do
-        SiteSetting.test_hello = true
-        SiteSetting.setting(:test_hello?, false)
-        SiteSetting.refresh!
-        SiteSetting.test_hello?.should_not == false
-      end
-    end
-  end
-
-  describe 'enum setting' do
-    before :all do
-      @enum_class = Class.new
-      SiteSetting.setting(:test_enum, 'en', enum: @enum_class)
-      SiteSetting.refresh!
-    end
-
-    it 'should have the correct default' do
-      expect(SiteSetting.test_enum).to eq('en')
-    end
-
-    context 'when overridden' do
-      after :each do
-        SiteSetting.remove_override!(:test_enum)
-      end
-
-      it 'stores valid values' do
-        @enum_class.expects(:valid_value?).with('fr').returns(true)
-        SiteSetting.test_enum = 'fr'
-        expect(SiteSetting.test_enum).to eq('fr')
-      end
-
-      it 'rejects invalid values' do
-        @enum_class.expects(:valid_value?).with('gg').returns(false)
-        expect { SiteSetting.test_enum = 'gg' }.to raise_error(Discourse::InvalidParameters)
-      end
-    end
-  end
-
   describe 'call_discourse_hub?' do
     it 'should be true when enforce_global_nicknames is true and discourse_org_access_key is set' do
       SiteSetting.enforce_global_nicknames = true
+      SiteSetting.enforce_global_nicknames.should == true
       SiteSetting.discourse_org_access_key = 'asdfasfsafd'
-      SiteSetting.refresh!
       SiteSetting.call_discourse_hub?.should == true
     end
 
     it 'should be false when enforce_global_nicknames is false and discourse_org_access_key is set' do
       SiteSetting.enforce_global_nicknames = false
       SiteSetting.discourse_org_access_key = 'asdfasfsafd'
-      SiteSetting.refresh!
       SiteSetting.call_discourse_hub?.should == false
     end
 
     it 'should be false when enforce_global_nicknames is true and discourse_org_access_key is not set' do
       SiteSetting.enforce_global_nicknames = true
       SiteSetting.discourse_org_access_key = ''
-      SiteSetting.refresh!
       SiteSetting.call_discourse_hub?.should == false
     end
 
     it 'should be false when enforce_global_nicknames is false and discourse_org_access_key is not set' do
       SiteSetting.enforce_global_nicknames = false
       SiteSetting.discourse_org_access_key = ''
-      SiteSetting.refresh!
       SiteSetting.call_discourse_hub?.should == false
     end
   end
@@ -194,4 +50,20 @@ describe SiteSetting do
     end
   end
 
+  describe 'in test we do some judo to ensure SiteSetting is always reset between tests' do
+
+    it 'is always the correct default' do
+      expect(SiteSetting.contact_email).to eq('')
+    end
+
+    it 'sets a setting' do
+      SiteSetting.contact_email = 'sam@sam.com'
+    end
+
+    it 'is always the correct default' do
+      expect(SiteSetting.contact_email).to eq('')
+    end
+
+  end
+
 end

spec/spec_helper.rb

@@ -59,6 +59,7 @@ Spork.prefork do
     config.fail_fast = ENV['RSPEC_FAIL_FAST'] == "1"
     config.include Helpers
     config.mock_framework = :mocha
+    config.order = 'random'
 
     # If you're not using ActiveRecord, or you'd prefer not to run each of your
     # examples within a transaction, remove the following line or assign false
@@ -77,10 +78,15 @@ Spork.prefork do
     config.before do
       # disable all observers, enable as needed during specs
       ActiveRecord::Base.observers.disable :all
+      SiteSetting.provider.all.each do |setting|
+        SiteSetting.remove_override!(setting.name)
+      end
     end
 
     config.before(:all) do
       DiscoursePluginRegistry.clear
+      require_dependency 'site_settings/local_process_provider'
+      SiteSetting.provider = SiteSettings::LocalProcessProvider.new
     end
 
   end
@@ -114,6 +120,7 @@ Spork.each_run do
   $redis.client.reconnect
   Rails.cache.reconnect
   MessageBus.after_fork
+
 end
 
 def build(*args)