Giving Roo's tests some love and attention

Gemfile

@@ -4,11 +4,9 @@ gemspec
 
 group :test do
   # additional testing libs
-  gem 'webmock'
   gem 'shoulda'
   gem 'activesupport', '< 5.1'
   gem 'rspec', '>= 3.0.0'
-  gem 'vcr'
   gem 'simplecov', '>= 0.9.0', require: false
   gem 'coveralls', require: false
 end

Gemfile_ruby2

@@ -6,13 +6,10 @@ gem 'nokogiri', "< 1.7.0"
 
 group :test do
   # additional testing libs
-  gem 'webmock'
   gem 'shoulda'
   gem 'rspec', '>= 3.0.0'
-  gem 'vcr'
   gem 'simplecov', '>= 0.9.0', require: false
   gem 'coveralls', require: false
-  # gem "pry"
   gem "activesupport", "~> 4.2.0"
 end
 
@@ -25,5 +22,6 @@ group :local_development do
   gem 'guard-preek', require: false
   gem 'guard-rubocop', require: false
   gem 'guard-reek', github: 'pericles/guard-reek', require: false
+  gem 'rb-readline'
   gem 'pry'
 end

lib/roo/base.rb

@@ -22,6 +22,10 @@ def self.TEMP_PREFIX
     Roo::TEMP_PREFIX
   end
 
+  def self.finalize(object_id)
+    proc { finalize_tempdirs(object_id) }
+  end
+
   def initialize(filename, options = {}, _file_warning = :error, _tmpdir = nil)
     @filename = filename
     @options = options

lib/roo/excelx.rb

@@ -4,12 +4,11 @@
 require 'roo/tempdir'
 require 'roo/utils'
 require 'forwardable'
+require 'set'
 
 module Roo
   class Excelx < Roo::Base
     extend Roo::Tempdir
-
-    require 'set'
     extend Forwardable
 
     ERROR_VALUES = %w(#N/A #REF! #NAME? #DIV/0! #NULL! #VALUE! #NUM!).to_set
@@ -46,7 +45,13 @@ def initialize(filename_or_stream, options = {})
         basename = find_basename(filename_or_stream)
       end
 
+      # NOTE: Create temp directory and allow Ruby to cleanup the temp directory
+      #       when the object is garbage collected. Initially, the finalizer was
+      #       created in the Roo::Tempdir module, but that led to a segfault
+      #       when testing in Ruby 2.4.0.
       @tmpdir = self.class.make_tempdir(self, basename, options[:tmpdir_root])
+      ObjectSpace.define_finalizer(self, self.class.finalize(object_id))
+
       @shared = Shared.new(@tmpdir)
       @filename = local_filename(filename_or_stream, @tmpdir, packed)
       process_zipfile(@filename || filename_or_stream)

lib/roo/open_office.rb

@@ -22,7 +22,12 @@ def initialize(filename, options = {})
 
       @only_visible_sheets = options[:only_visible_sheets]
       file_type_check(filename, '.ods', 'an Roo::OpenOffice', file_warning, packed)
-      @tmpdir   = self.class.make_tempdir(self, find_basename(filename), options[:tmpdir_root])
+      # NOTE: Create temp directory and allow Ruby to cleanup the temp directory
+      #       when the object is garbage collected. Initially, the finalizer was
+      #       created in the Roo::Tempdir module, but that led to a segfault
+      #       when testing in Ruby 2.4.0.
+      @tmpdir = self.class.make_tempdir(self, find_basename(filename), options[:tmpdir_root])
+      ObjectSpace.define_finalizer(self, self.class.finalize(object_id))
       @filename = local_filename(filename, @tmpdir, packed)
       # TODO: @cells_read[:default] = false
       open_oo_file(options)

lib/roo/tempdir.rb

@@ -2,24 +2,19 @@ module Roo
   module Tempdir
     def finalize_tempdirs(object_id)
       if @tempdirs && (dirs_to_remove = @tempdirs[object_id])
-        @tempdirs[object_id] = nil
+        @tempdirs.delete(object_id)
         dirs_to_remove.each do |dir|
           ::FileUtils.remove_entry(dir)
         end
       end
     end
 
     def make_tempdir(object, prefix, root)
-      root ||= ENV['ROO_TMP']
-      # folder is cleaned up in .finalize_tempdirs
+      root ||= ENV["ROO_TMP"]
+      # NOTE: This folder is cleaned up by finalize_tempdirs.
       ::Dir.mktmpdir("#{Roo::TEMP_PREFIX}#{prefix}", root).tap do |tmpdir|
-        @tempdirs ||= {}
-        if @tempdirs[object.object_id]
-          @tempdirs[object.object_id] << tmpdir
-        else
-          @tempdirs[object.object_id] = [tmpdir]
-          ObjectSpace.define_finalizer(object, method(:finalize_tempdirs))
-        end
+        @tempdirs ||= Hash.new { |h, k| h[k] = [] }
+        @tempdirs[object.object_id] << tmpdir
       end
     end
   end

roo.gemspec

@@ -22,4 +22,5 @@ Gem::Specification.new do |spec|
 
   spec.add_development_dependency 'rake', '~> 10.1'
   spec.add_development_dependency 'minitest', '~> 5.4', '>= 5.4.3'
+  spec.add_development_dependency 'rack', '~> 1.6', '< 2.0.0'
 end

spec/spec_helper.rb

@@ -1,13 +1,7 @@
 require 'simplecov'
 require 'roo'
-require 'vcr'
 require 'helpers'
 
 RSpec.configure do |c|
   c.include Helpers
 end
-
-VCR.configure do |c|
-  c.cassette_library_dir = 'spec/fixtures/vcr_cassettes'
-  c.hook_into :webmock # or :fakeweb
-end

test/roo/test_csv.rb

@@ -0,0 +1,39 @@
+require 'test_helper'
+
+class TestRooCSV < Minitest::Test
+  def test_sheets
+    file = filename("numbers1")
+    workbook = roo_class.new(File.join(TESTDIR, file))
+    assert_equal ["default"], workbook.sheets
+    assert_raises(RangeError) { workbook.default_sheet = "no_sheet" }
+    assert_raises(TypeError)  { workbook.default_sheet = [1, 2, 3] }
+    workbook.sheets.each do |sh|
+      workbook.default_sheet = sh
+      assert_equal sh, workbook.default_sheet
+    end
+  end
+
+  def test_nil_rows_and_lines_csv
+    # x_123
+    oo = Roo::CSV.new(File.join(TESTDIR,'Bibelbund.csv'))
+    oo.default_sheet = oo.sheets.first
+    assert_equal 1, oo.first_row
+  end
+
+  def test_csv_parsing_with_headers
+    return unless CSV
+    headers = ["TITEL", "VERFASSER", "OBJEKT", "NUMMER", "SEITE", "INTERNET", "PC", "KENNUNG"]
+
+    oo = Roo::Spreadsheet.open(File.join(TESTDIR, "Bibelbund.csv"))
+    parsed = oo.parse(headers: true)
+    assert_equal headers, parsed[1].keys
+  end
+
+  def roo_class
+    Roo::CSV
+  end
+
+  def filename(name)
+    "#{name}.csv"
+  end
+end

test/roo/test_excelx.rb

@@ -0,0 +1,186 @@
+require "test_helper"
+
+class TestRooExcelx < Minitest::Test
+  def test_download_uri_with_invalid_host
+    assert_raises(RuntimeError) do
+      Roo::Excelx.new("http://example.com/file.xlsx")
+    end
+  end
+
+  def test_download_uri_with_query_string
+    file = filename("simple_spreadsheet")
+    port = 12_344
+    url = "#{local_server(port)}/#{file}?query-param=value"
+
+    start_local_server(file, port) do
+      spreadsheet = roo_class.new(url)
+      assert_equal "Task 1", spreadsheet.cell("f", 4)
+    end
+  end
+
+  def test_should_raise_file_not_found_error
+    assert_raises(IOError) do
+      Roo::Excelx.new(File.join("testnichtvorhanden", "Bibelbund.xlsx"))
+    end
+  end
+
+  def test_file_warning_default
+    assert_raises(TypeError) { Roo::Excelx.new(File.join(TESTDIR, "numbers1.ods")) }
+    assert_raises(TypeError) { Roo::Excelx.new(File.join(TESTDIR, "numbers1.xls")) }
+  end
+
+  def test_file_warning_error
+    %w(ods xls).each do |extension|
+      assert_raises(TypeError) do
+        options = { packed: false, file_warning: :error }
+        Roo::Excelx.new(File.join(TESTDIR, "numbers1. #{extension}"), options)
+      end
+    end
+  end
+
+  def test_file_warning_warning
+    options = { packed: false, file_warning: :warning }
+    assert_raises(ArgumentError) do
+      Roo::Excelx.new(File.join(TESTDIR, "numbers1.ods"), options)
+    end
+  end
+
+  def test_file_warning_ignore
+    options = { packed: false, file_warning: :ignore }
+    sheet = Roo::Excelx.new(File.join(TESTDIR, "type_excelx.ods"), options)
+    assert sheet, "Should not throw an error"
+  end
+
+  def test_bug_xlsx_reference_cell
+    # NOTE: If cell A contains a string and cell B references cell A.  When
+    #       reading the value of cell B, the result will be "0.0" instead of the
+    #       value of cell A.
+    #
+    # Before this test case, the following code:
+    #
+    # spreadsheet = Roo::Excelx.new("formula_string_error.xlsx")
+    # spreadsheet.default_sheet = "sheet1"
+    # p "A: #{spreadsheet.cell(1, 1)}" #=> "A: TestString"
+    # p "B: #{spreadsheet.cell(2, 1)}" #=> "B: 0.0"
+    #
+    # where the expected result is
+    # "A: TestString"
+    # "B: TestString"
+    xlsx = Roo::Excelx.new(File.join(TESTDIR, "formula_string_error.xlsx"))
+    assert_equal "Teststring", xlsx.cell("a", 1)
+    assert_equal "Teststring", xlsx.cell("a", 2)
+  end
+
+  def test_parsing_xslx_from_numbers
+    xlsx = Roo::Excelx.new(File.join(TESTDIR, "numbers-export.xlsx"))
+
+    xlsx.default_sheet = xlsx.sheets.first
+    assert_equal "Sheet 1", xlsx.cell("a", 1)
+
+    # Another buggy behavior of Numbers 3.1: if a warkbook has more than a
+    # single sheet, all sheets except the first one will have an extra row and
+    # column added to the beginning. That's why we assert against cell B2 and
+    # not A1
+    xlsx.default_sheet = xlsx.sheets.last
+    assert_equal "Sheet 2", xlsx.cell("b", 2)
+  end
+
+  def assert_cell_range_values(sheet, row_range, column_range, is_merged_range, expected_value)
+    row_range.each do |row|
+      column_range.each do |col|
+        value = sheet.cell(col, row)
+        if is_merged_range.call(row, col)
+          assert_equal expected_value, value
+        else
+          assert_nil value
+        end
+      end
+    end
+  end
+
+  def test_expand_merged_range
+    options = { expand_merged_ranges: true }
+    xlsx = Roo::Excelx.new(File.join(TESTDIR, "merged_ranges.xlsx"), options)
+
+    [
+      {
+        rows: (3..7),
+        columns: ("a".."b"),
+        conditional: ->(row, col) { row > 3 && row < 7 && col == "a" },
+        expected_value: "vertical1"
+      },
+      {
+        rows: (3..11),
+        columns: ("f".."h"),
+        conditional: ->(row, col) { row > 3 && row < 11 && col == "g" },
+        expected_value: "vertical2"
+      },
+      {
+        rows: (3..5),
+        columns: ("b".."f"),
+        conditional: ->(row, col) { row == 4 && col > "b" && col < "f" },
+        expected_value: "horizontal"
+      },
+      {
+        rows: (8..13),
+        columns: ("a".."e"),
+        conditional: ->(row, col) { row > 8 && row < 13 && col > "a" && col < "e" },
+        expected_value: "block"
+      }
+    ].each do |data|
+      rows, cols, conditional, expected_value = data.values
+      assert_cell_range_values(xlsx, rows, cols, conditional, expected_value)
+    end
+  end
+
+  def test_noexpand_merged_range
+    xlsx = Roo::Excelx.new(File.join(TESTDIR, "merged_ranges.xlsx"))
+
+    [
+      {
+        rows: (3..7),
+        columns: ("a".."b"),
+        conditional: ->(row, col) { row == 4 && col == "a" },
+        expected_value: "vertical1"
+      },
+      {
+        rows: (3..11),
+        columns: ("f".."h"),
+        conditional: ->(row, col) { row == 4 && col == "g" },
+        expected_value: "vertical2"
+      },
+      {
+        rows: (3..5),
+        columns: ("b".."f"),
+        conditional: ->(row, col) { row == 4 && col == "c" },
+        expected_value: "horizontal"
+      },
+      {
+        rows: (8..13),
+        columns: ("a".."e"),
+        conditional: ->(row, col) { row == 9 && col == "b" },
+        expected_value: "block"
+      }
+    ].each do |data|
+      rows, cols, conditional, expected_value = data.values
+      assert_cell_range_values(xlsx, rows, cols, conditional, expected_value)
+    end
+  end
+
+  def test_open_stream
+    file = filename(:numbers1)
+    file_contents = File.read File.join(TESTDIR, file), encoding: "BINARY"
+    stream = StringIO.new(file_contents)
+    xlsx = Roo::Excelx.new(stream)
+    expected_sheet_names = ["Tabelle1", "Name of Sheet 2", "Sheet3", "Sheet4", "Sheet5"]
+    assert_equal expected_sheet_names, xlsx.sheets
+  end
+
+  def roo_class
+    Roo::Excelx
+  end
+
+  def filename(name)
+    "#{name}.xlsx"
+  end
+end

test/roo/test_libre_office.rb

@@ -0,0 +1,9 @@
+require "test_helper"
+
+class TestRooOpenOffice < Minitest::Test
+  def test_libre_office
+    oo = Roo::LibreOffice.new(File.join(TESTDIR, "numbers1.ods"))
+    oo.default_sheet = oo.sheets.first
+    assert_equal 41, oo.cell("a", 12)
+  end
+end