cleaning-up-ui-race-conditions.md

An Example of Getting Rid of Dead Waiting

Consider the following spec which as intermittently failing the should update savings spec.

# spec/integration/drafts/pro/finishing_spec.rb

require 'spec_helper'

describe 'finishing', js: true do
  before(:each) do
    initialize_order_environment!

    @user = FactoryGirl.create(:user, :iw_test_user)
    @admin = FactoryGirl.create(:administrator)
  end

  context 'crease only' do
    before(:each) do
      login(@admin, then_visit: '/drafts/new/pro/1/job_type')
      wait_for_ajax
      find('#tp_post_card_7_x_5').click
      wait_for_ajax
      find('.tp_finishing').click
      wait_for_ajax
      first(:xpath, "//*[text()='Single Fold']").click
      wait_for_ajax
    end

    it 'should show crease/fold buttons' do
      expect(page).to have_css('.tp-choose-creased')
    end

    it 'should update savings' do
      savings = (match = find('.tp-crease-savings').text.match(/\$(\d+\.\d+)/)) && match[1]&.to_f

      fill_in('quantity', with: 100)
      wait_for_ajax

      new_savings = (match = find('.tp-crease-savings').text.match(/\$(\d+\.\d+)/)) && match[1]&.to_f

      expect(new_savings).to be > savings
    end
  end
end

First - Use wait_for and get rid of the other asynchronous waits

require 'spec_helper'

describe 'finishing', js: true do
  before(:each) do
    initialize_order_environment!

    @user = FactoryGirl.create(:user, :iw_test_user)
    @admin = FactoryGirl.create(:administrator)
  end

  context 'crease only' do
    before(:each) do
      # use rack_session for quicker loading
      login(@admin, then_visit: '/drafts/new/pro/1/job_type', rack_session: true)
      # wait_for_ajax useless! find will wait until tp_post_card_7_x_5 is visible
      find('#tp_post_card_7_x_5').click  
      # remove useless wait_for_ajax 
      find('.tp_finishing').click
      # remove useless wait_for_ajax
      first(:xpath, "//*[text()='Single Fold']").click
    end

    it 'should show crease/fold buttons' do
      expect(page).to have_selector('.tp-choose-creased')
    end

    it 'should update savings' do
      savings = (match = find('.tp-crease-savings').text.match(/\$(\d+\.\d+)/)) && match[1]&.to_f

      fill_in('quantity', with: 100)

      # wait_for_ajax.  Well we do need to wait, but instead of waiting for ajax lets just wait 
      # for the data to change as we expect to, using wait_for

      wait_for { (match = find('.tp-crease-savings').text.match(/\$(\d+\.\d+)/)) && match[1]&.to_f }
        .to be > savings
    end
  end
end

The key here is that not only is wait_for_ajax useless, it can actually hide problems. The user will not wait, so why should the spec?

And as an added benefit removing the wait_for_ajax calls speeds things up by about 10% since the spec will continue the instant the css is available.

Spec now ALWAYS FAILS!

Now that we have removed the wait for ajax calls, we actually expose the reason this spec was intermittently failing.

This line

first(:xpath, "//*[text()='Single Fold']").click

will successfully execute as soon as Single Fold is visible even if its disabled. The wait_for_ajax was masking this, by waiting until the system appeared to settle down.

If we change that line to:

find("#finishing_option_#{FinishingOption.find_by_name('Single Fold').id}").find(:css, '.option-div:not(.disabled)').click

it should work, simply by waiting until the option is not disabled.

But it does not!

The problem is that as the finishing options become enabled BEFORE the screen finishes updating, but then are disabled, the screen is updated again, and finally the default value (No Finishing) is selected. So if you click on the Single Fold item the first time it is enabled, the selection gets erased when the No Finishing is selected.

DONT CHEAT!

The first reaction might be, to wait till you see No Finishing selected before selecting Single Fold.

Instead lets get to root cause of the problem.

The Root Cause

Have a look at this code:

# app/hyperstack/components/drafts/pro/sidebar/finishing/finishing_option_item.rb

module Components
  module Drafts
    module Pro
      module Sidebar
        class FinishingOptionItem < ApplicationComponent
          ...
          def conflicts
            return false if @Job.calculate_conflict_triggers.loading?

            @Job.calculate_conflict_triggers[:finishing_option_ids].any? &&
              @Job.calculate_conflict_triggers[:finishing_option_ids].map { |k, v| [k.to_i, v] }.to_h
          end

          def selected?
            !!@Selected
          end

          def conflicting?
            !selected? && conflicts && conflicts.any? && conflicts.keys.include?(@FinishingOption.id)
          end

          def option_div_classes
            ['option-div'].tap do |classes|
              classes << 'selected' if selected?
              classes << 'disabled' if conflicting?
            end
          end
          ...
        end 
      end 
    end 
  end 
end

If the data is loading, then the system assumes there are no conflicts, and the options are enabled.

The fix is to have conflicts return a valid "dummy" conflict instead of acting like there a NO conflicts. This keeps all selections disabled until all the data is actually loaded. The UI looks better too (less flicker)

  def conflicts
    return { @FinishingOption.id => 'loading...' } if @Job.calculate_conflict_triggers.loading?
    ...
  end 

Mitch's rule number 27 (based on patent 4951069 ) While waiting for data, its always best to assume the worst case scenario.