Column mapping dangerously applied twice to the same cell!

[jbrains]

Wow. This was fun. I don't know how this ever worked. By coincidence, are all the conversion procs that everyone has ever used idempotent?! That would surprise me.

Here's the minimal failing spec (why Money matters will be apparent soon):

gem 'money'
require 'money'

describe "Mapping a column" do
  example "my conversion proc is called twice, and that sucks" do
    @table = Cucumber::Ast::Table.new([["Receipt Amount"], ["CAD 100"]])
    conversion_proc_for_receipt_amount = mock()
    conversion_proc_for_receipt_amount.should_receive(:call).once.and_return(Money.new(10000, "CAD"))
    @table.map_column!("Receipt Amount") { |text| conversion_proc_for_receipt_amount.call }
    @table.hashes.should == [{"Receipt Amount" => Money.new(10000, "CAD")}]
  end
end

This spec fails, because it calls my column mapping proc twice. For example, this causes Table to try to parse "CAD 100" into a Money, then parse that Money object again, which happens to do something weird and annoying. Clearly, that ain't right. Without some weird hack, like refusing to parse something that's already a Money, I don't know how to make that conversion proc idempotent. That would seem like an unreasonable requirement.

The two calls appear to be in convert_columns! and to_hash. Without digging deeper, it seems to me that to_hash ought not to convert the columns directly, when it could use convert_columns!, and that method could memoize the result.

I don't have the energy to fix this now, and even so, I'd like someone there to confirm that it's indeed a mistake, and that you didn't intend for the conversion procs to need to be idempotent.

Thanks!

[CoryFoy]

Hi J.B.,

I didn't get the failure you did. I used the following:

require 'cucumber'
describe "Mapping a column" do
        example "this works" do
          @table = Cucumber::Ast::Table.new([["Receipt Amount"], ["CAD 100"]])
          conv_proc = mock()
          conv_proc.should_receive(:call).once.and_return(100)
          @table.map_column!("Receipt Amount") { |text| conv_proc.call }
          @table.hashes.should == [{"Receipt Amount" => 100}]
        end
end

Even installing a money gem passes:

require 'cucumber'
gem 'money'
require 'money'
describe "Mapping a column" do
        example "this works" do
          @table = Cucumber::Ast::Table.new([["Receipt Amount"], ["CAD 100"]])
          conv_proc = mock()
          conv_proc.should_receive(:call).once.and_return(Money.new(10000, "CAD"))
          @table.map_column!("Receipt Amount") { |text| conv_proc.call }
          @table.hashes.should == [{"Receipt Amount" => Money.new(10000, "CAD")}]
        end
end

corys-mac-mini:jbcuke coryfoy$ rspec spec/
.

Finished in 1.74 seconds
1 example, 0 failures


corys-mac-mini:jbcuke coryfoy$ cucumber --version
1.1.0
corys-mac-mini:jbcuke coryfoy$ ruby -v
ruby 1.9.2p290 (2011-07-09 revision 32553) [x86_64-darwin11.0.1]

Yikes! Out of date cucumber. Installed 1.1.4 and got...failure!

corys-mac-mini:jbcuke coryfoy$ cucumber --version
1.1.4
corys-mac-mini:jbcuke coryfoy$ rspec spec/
F

Failures:

  1) Mapping a column this works
     Failure/Error: conv_proc.should_receive(:call).once.and_return(Money.new(10000, "CAD"))
       (Mock).call(any args)
           expected: 1 time
           received: 2 times
     # ./spec/money_spec.rb:8:in `block (2 levels) in <top (required)>'

Finished in 1.58 seconds
1 example, 1 failure

Failed examples:

rspec ./spec/money_spec.rb:5 # Mapping a column this works

Looks like the bug was introduced here possibly? cucumber/common@abb0025#diff-0

[jbrains]

git bisect concurs that this commit introduced the difference: cucumber/common@abb0025

Thank you for helping nail that down, at least.

[jbrains]

It looks like that commit had a failing spec. Tsk tsk.

[mattwynne]

Thanks for reporting this JB. I've never used this feature but it sounds as if the behaviour you expected was reasonable.

[jbrains]

I've exchange email with Richard; I might or might not look into this myself to try to fix it. I would like to, but other priorities have crowded it out for now.

[Mandaryn]

A quick workaround until this is fixed (check if text is a String)

@table.map_column!("Receipt Amount") { |text| do_mumbo_jumbo(text) if text.is_a?(String) }

[briandunn]

Except when text is not a String your map is now returning nil. that doesn't sound useful. perhaps
{ |text| text.is_a?(String) ? do_mumbo_jumbo(text) : text }
Or just #208.

[Mandaryn]

yep, my trying to be a smartass here :). I really do in my code what you suggested above... thx for clarifying this

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.