1

I am trying to test the following method, which given a base64 string, it prints a PNG:

def self.decode64_to_png(src)
    if (src.start_with? "data:image/png;base64")
      png = Base64.decode64(src['data:image/png;base64,'.length .. -1])
      png_logo_uuid = SecureRandom.urlsafe_base64(nil,false)
      if Rails.env.production?
        src = "#{Rails.root}/tmp/#{png_logo_uuid}.png" 
        File.open(src, 'wb') { |f| f.write(png) }
      else
        src = "#{Rails.root}/public/images/#{png_logo_uuid}.png" 
        File.open(src, 'wb') { |f| f.write(png) }
      end
      return src 
    end
  end

But I am pretty new to testing and I am trying to figure out what would be the best way to test this in rspec. I know I should have written the test first, but I am still learning. Bu basically I want to make sure that:

1 - Given a correct base64 string, an image is created and src is returned
2 - Given an incorrect base64, nil is returned.

This is the test that I have done so far:

describe ".decode64_to_png" do
    it 'returns a path if the source is a correct base64 string' do
      File.stub(:open) { true }
      src = "data:image/png;base64,iVBORw0KGg"
      ImageManipulation.decode64_to_png(src).should_not be_nil
    end

    it 'returns nil if the source is not a correct base64 string' do
      File.stub(:open) { true }
      src = "asdasdasdata:image/png;base64,iVBORw0KGg"
      ImageManipulation.decode64_to_png(src).should be_nil
    end
  end
Hommer Smith
  • 26,772
  • 56
  • 167
  • 296
  • Not related to your question, but a design which returns `nil` on error may cause you problems in future. It would be better to raise an error, and if you have a situation where flow is supposed to continue regardless then use `begin ... rescue` to wrap just that call. The question is an interesting test situation regardless (in essence your method has a very important side-effect not shown by the return value, and you want to assert that it has happened) – Neil Slater May 16 '14 at 12:57
  • Actually, the return value is the new path, so you *can* test it. Worse problem is `if Rails.env.production?` - you really need to get rid of that otherwise your test will be meaningless – Neil Slater May 16 '14 at 13:14
  • Could you add your existing test, so it is clear how much rspec you already know, and where you are stuck? – Neil Slater May 16 '14 at 13:35
  • Added the test that I have now... Neil, why is Rails.env.production? a problem? In my production site I can't write to public/images, that's why I use /tmp... – Hommer Smith May 16 '14 at 17:29
  • The branching *only when in production* places the code you want to test out of reach. You happen to of placed similar code next to it in the same method. But you test *that* code not the code you will then use. That makes the test almost pointless (it tests code you only run in dev and test mode, not the code you actually need to be working). The way to solve this is to use *configuration* to decide where to save the files, and have one code path, then it can be tested and keep the files used for testing separate. – Neil Slater May 16 '14 at 23:20
  • @NeilSlater thanks for your suggestion. Could you explain what do you mean by 'configuration'? – Hommer Smith May 20 '14 at 17:55

1 Answers1

1

untested

describe ".decode64_to_png" do
  let(:payload) { "iVBORw0KGg" }
  let(:uuid) { "uuid" }
  let(:file_args) { "#{Rails.root}/tmp/uuid.png" }

  context "when src is valid" do
    let(:src) { "data:image/png;base64,#{payload}" }
    before do
      Base64.should_receive(:decode64).with(payload)
      SecureRandom.should_receive(:urlsafe_base64).with(nil, false).and_return(uuid)
      Rails.stub_chain(:env, :production?) { true }
    end
    it "writes the image to a file" do
      File.should_receive(:open).with(file_args)
      ImageManipulation.decode64_to_png(src)
    end
    it "returns the file path" do
      File.stub(:open)
      ImageManipulation.decode64_to_png(src).should eq(file_args)
    end
  end

  context "when src is invalid" do
    let(:src) { "asdadasdada:data:image/png;base64,#{payload}" }
    it "does not write the image" do
      File.should_not_receive(:open)
      ImageManipulation.decode64_to_png(src)
    end
    it "returns nil" do
      ImageManipulation.decode64_to_png(src).should be_nil
    end
  end
end

Testing the side effect (i.e., the file was written) seems more important here than the return value; you should test for both.

Note that the last example fails, because when the input is invalid the method returns the input, instead of returning nil. I'd argue the method should raise an exception instead.

Note also that this only tests the code path for Rails.env.production? being true. You'd be better off moving that out of the method entirely and passing in the root portion of the path to the method, then handling the difference between environments in the calling code, or in an initializer.

How would you rewrite the function so that the test makes sense in all environments?

There are several ways; you could add a Settings class and stick the image folder path in it when initializing:

# lib/settings.rb
class Settings
  include Singleton
  attr_accessor :image_path
end

# config/initializers/settings.rb
case Rails.env
when "production"
  Settings.instance.image_path = "#{Rails.root}/tmp"
else
  Settings.instance.image_path = "#{Rails.root}/public/images"
end

replace

if Rails.env.production?
  src = "#{Rails.root}/tmp/#{png_logo_uuid}.png" 
  File.open(src, 'wb') { |f| f.write(png) }
else
  src = "#{Rails.root}/public/images/#{png_logo_uuid}.png" 
  File.open(src, 'wb') { |f| f.write(png) }
end

with

src = File.join(Settings.instance.image_path, "#{png_logo_uuid}.png")

add to the spec

Settings.stub_chain(:instance, image_path) { "#{Rails.root}/tmp" }

You could also read the values from a YAML file as described here, or use a gem like simpleconfig.

Community
  • 1
  • 1
zetetic
  • 47,184
  • 10
  • 111
  • 119
  • zetetic, I don't understand the part of diferentiating between Rails.env.production? .How would you rewrite the function so that the test makes sense in all environments? – Hommer Smith May 20 '14 at 17:56