Two bugs with Vanity and functional tests #37

Closed
dlindahl opened this Issue Feb 15, 2011 · 1 comment

Projects

None yet

2 participants

@dlindahl

I believe I have found two bugs with Vanity when it is running inside a functional test. Before I submit a Pull Request, I wanted to make sure that what I have found are indeed bugs and not just an test implementation issue.

First off, my test looks something like this:

context "A GET to :foo" do
  context "while the 'bar_3' Vanity experiment alternative is chosen" do
    setup do
      Vanity.playground.experiment(:my_experiment).chooses('bar_3')
      get :foo
    end
    should_render "my_controller/foo/bar_3"
  end
end

I ran into two problems with this test case.

1) in lib/vanity/frameworks/rails.rb, the use_vanity method initially sets @vanity_identity to "test" which is later used to store the index of the alternative which has been specifically chosen ("bar_3" in the above example)

The problem occurs once the Controller Action is executed. the use_vanity method now sees a response object, so it then generates a new @vanity_identity which will then default to an entirely different test alternative index.

Adding an additional test to see if there is a response and it is not an instance of ActionController::TestResponse allowed Vanity to correctly set the @vanity_identity every time.

The real problem might be that @vanity_identity seems to be losing its value at some point during execution. Not sure where that would be occurring and the vanity_identity method was run once... so I'm sort of lost on that one.

def use_vanity(symbol = nil, &block)
  if block
    define_method(:vanity_identity) { block.call(self) }
  else
    define_method :vanity_identity do
      return @vanity_identity if @vanity_identity
      if symbol && object = send(symbol)
        @vanity_identity = object.id
      elsif response and not response.is_a?(ActionController::TestResponse) # everyday use
        @vanity_identity = cookies["vanity_id"] || ActiveSupport::SecureRandom.hex(16)
        cookies["vanity_id"] = { :value=>@vanity_identity, :expires=>1.month.from_now }
        @vanity_identity
      else # during functional testing
        @vanity_identity = "test"
      end
    end
  end
  around_filter :vanity_context_filter
  before_filter :vanity_reload_filter unless ::Rails.configuration.cache_classes
  before_filter :vanity_query_parameter_filter
end

2) in lib/vanity/experiment/ab_test.rb, the choose method is correctly choosing an alternative to display, but it is never saving that value to the index variable for use later on in the method. That means that it will always choose the first test (since nil.to_i == 0) which is obviously not what was explicitly chosen by the chooses method.

Adding a simple variable assignment resolved that issue.

def choose
  if @playground.collecting?
    # snip #
  else
    identity = identity()
    @showing ||= {}
    @showing[identity] ||= alternative_for(identity)
    index = @showing[identity] # Assign showing value for the current identity to index so the correct Alt. can be displayed
  end
  @alternatives[index.to_i]
end

Hopefully this was helpful. Please let me know if anymore information is needed to reproduce the bugs, I will be more than happy to provide what I can.

If the above looks good, I will submit a Pull Request once I double check Vanity's tests.

@phillbaker phillbaker added the testing label Jun 8, 2014
@phillbaker
Collaborator

Closing, as seems related to #209 and hasn't been updated in years.

@phillbaker phillbaker closed this Jul 20, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment