CODEWITHSUNDEEP
ruby-on-railsunit-testingrspecstub
Caco
Caco

Reputation: 1654

Rspec: Tempfile subclass of File get intercepted by File stub request

I'm doing a stub request to File but since I call Tempfile (that is a subclass of File) before calling File, Tempfile is intercepting the stub I define.

Model:

def download_file
  #...
  begin
    tempfile = Tempfile.new([upload_file_name, '.csv'])
    File.open(tempfile, 'wb') { |f| f.write(result.body.to_s) }
    tempfile.path
  rescue Errno::ENOENT => e
    puts "Error writing to file: #{e.message}"
    e
  end
end

Rspec example:

it 'could not write to tempfile, so the report is created but without content' do
  allow(File).to receive(:open).and_return Errno::ENOENT
  response_code = post @url, params: { file_ids: [@file.id] }
  expect(response_code).to eql(200)
  assert_requested :get, /#{@s3_domain}.*/, body: @body, times: 1

  report = @file.frictionless_report
  expect(report.report).to eq(nil)
end

Error:

tempfile in the line

tempfile = Tempfile.new([upload_file_name, '.csv'])

receives Errno::ENOENT, indicating that the stub is going to Tempfile instead of to File.

How can I define the stub to go to File instead of to Tempfile?

Upvotes: 0

Views: 1313

Answers (1)

Schwern
Schwern

Reputation: 165004

There's no need to reopen a Tempfile, it's already open and delegates to File.

def download_file
  tempfile = Tempfile.new([upload_file_name, '.csv'])
  tempfile.write(result.body.to_s)
  tempfile.path
# A method has an implicit begin.
rescue Errno::ENOENT => e
  puts "Error writing to file: #{e.message}"
  e
end

Then you can mock just Tempfile.new. Note that exceptions are raised, not returned.

it 'could not write to tempfile, so the report is created but without content' do
  # Exceptions are raised, not returned.
  allow(Tempfile).to receive(:new)
    .and_raise Errno::ENOENT

  response_code = post @url, params: { file_ids: [@file.id] }
  expect(response_code).to eql(200)
  assert_requested :get, /#{@s3_domain}.*/, body: @body, times: 1

  report = @file.frictionless_report
  expect(report.report).to eq(nil)
end

However, this remains fragile glass-box testing. Your test has knowledge of the implementation, if the implementation changes the test gives a false negative. And it still has to hope mocking Tempfile.new doesn't break something else.

Instead, extract temp file creation from download_file.

private def new_temp_file_for_upload
  Tempfile.new([upload_file_name, '.csv'])
end

def download_file
  tempfile = new_temp_file_for_upload
  tempfile.write(result.body.to_s)
  tempfile.path
rescue Errno::ENOENT => e
  puts "Error writing to file: #{e.message}"
  e
end

Now the mocking can be targeted to a specific method in a specific object. And we can apply some good rspec patterns.

context 'when the Tempfile cannot be created' do
  # Here I'm assuming download_file is part of the Controller being tested.
  before do
    allow(@controller).to receive(:new_temp_file_for_upload)
      .and_raise Errno::ENOENT
  end

  it 'creates the report without content' do
    post @url, params: { file_ids: [@file.id] }

    expect(response).to have_http_status(:success)
    assert_requested :get, /#{@s3_domain}.*/, body: @body, times: 1

    report = @file.frictionless_report
    expect(report.report).to be nil
  end
end

Note: returning "success" and an empty report after an internal failure is probably incorrect. It should return a 5xx error so the user knows there was a failure without having to look at the content.

download_file is doing too many things. It's both downloading a file and deciding what to do with a specific error. It should just download the file. Let something higher up in the call stack decide what to do with the exception. Methods which do one thing are simpler and more flexible and easier to test and less buggy.

private def new_temp_file_for_upload
  Tempfile.new([upload_file_name, '.csv'])
end

def download_file
  tempfile = new_temp_file_for_upload
  tempfile.write(result.body.to_s)
  tempfile.path
end

context 'when the download fails' do
  before do
    allow(@controller).to receive(:download_file)
      .and_raise "krunch!"
  end

  it 'responds with an error' do
    post @url, params: { file_ids: [@file.id] }

    expect(response).to have_http_status(:error)
  end
end

Note that no specific error is needed. It's enough that download_file raises an exception. This test now has no knowledge of the internals beyond knowing that download_file is called.

Upvotes: 2

Related Questions