From 87fc615d83710d854b3c2485aca9d3fb2b377b77 Mon Sep 17 00:00:00 2001
From: Phil Ross <phil.ross@gmail.com>
Date: Sat, 21 May 2016 19:48:16 +0100
Subject: [PATCH] Improvements to the reporting of invalid identifiers.

- Include the identifier in all error cases in
  ZoneinfoDataSource#load_timezone_info.
- Include the country code in InvalidCountryCode messages raised in
  ZoneinfoDataSource#load_country_info (this was already being done in
  the RubyDatasSource equivalent).
- Minor changes to the format of the exception messages.
- Add additional test cases.
- Make tests match on word boundaries to avoid matching filenames, etc
  that may also be included in the message.
---
 lib/tzinfo/ruby_data_source.rb     |  6 ++---
 lib/tzinfo/zoneinfo_data_source.rb | 10 ++++----
 test/tc_country.rb                 | 28 ++++++++--------------
 test/tc_ruby_data_source.rb        | 18 +++++++++-----
 test/tc_timezone.rb                | 16 ++++++-------
 test/tc_timezone_proxy.rb          | 25 ++++++++++++--------
 test/tc_zoneinfo_data_source.rb    | 38 ++++++++++++++++++++----------
 7 files changed, 78 insertions(+), 63 deletions(-)

diff --git a/lib/tzinfo/ruby_data_source.rb b/lib/tzinfo/ruby_data_source.rb
index 3671533e..08e786a3 100644
--- a/lib/tzinfo/ruby_data_source.rb
+++ b/lib/tzinfo/ruby_data_source.rb
@@ -20,7 +20,7 @@ class RubyDataSource < DataSource
     # identifier is invalid.
     def load_timezone_info(identifier)
       original_identifier = identifier
-      raise InvalidTimezoneIdentifier, "Invalid identifier - #{original_identifier}" if identifier !~ /^[A-Za-z0-9\+\-_]+(\/[A-Za-z0-9\+\-_]+)*$/
+      raise InvalidTimezoneIdentifier, "Invalid identifier: #{original_identifier}" if identifier !~ /^[A-Za-z0-9\+\-_]+(\/[A-Za-z0-9\+\-_]+)*$/
 
       identifier = identifier.gsub(/-/, '__m__').gsub(/\+/, '__p__')
 
@@ -40,7 +40,7 @@ def load_timezone_info(identifier)
 
         m.get
       rescue LoadError, NameError => e
-        raise InvalidTimezoneIdentifier, "#{e.message} - #{original_identifier}"
+        raise InvalidTimezoneIdentifier, "#{e.message} (loading #{original_identifier})"
       end
     end
 
@@ -70,7 +70,7 @@ def linked_timezone_identifiers
     def load_country_info(code)
       load_country_index
       info = Data::Indexes::Countries.countries[code]
-      raise InvalidCountryCode, "Invalid country code - #{code}" unless info
+      raise InvalidCountryCode, "Invalid country code: #{code}" unless info
       info
     end
 
diff --git a/lib/tzinfo/zoneinfo_data_source.rb b/lib/tzinfo/zoneinfo_data_source.rb
index 12a71106..cb9a74ab 100644
--- a/lib/tzinfo/zoneinfo_data_source.rb
+++ b/lib/tzinfo/zoneinfo_data_source.rb
@@ -202,15 +202,15 @@ def load_timezone_info(identifier)
           begin
             ZoneinfoTimezoneInfo.new(identifier, path)
           rescue InvalidZoneinfoFile => e
-            raise InvalidTimezoneIdentifier, e.message
+            raise InvalidTimezoneIdentifier, "#{e.message} (loading #{identifier})"
           end
         else
-          raise InvalidTimezoneIdentifier, "Invalid identifier - #{identifier}"
+          raise InvalidTimezoneIdentifier, "Invalid identifier: #{identifier}"
         end
       rescue Errno::ENOENT, Errno::ENAMETOOLONG, Errno::ENOTDIR
-        raise InvalidTimezoneIdentifier, "Invalid identifier - #{identifier}"
+        raise InvalidTimezoneIdentifier, "Invalid identifier: #{identifier}"
       rescue Errno::EACCES => e
-        raise InvalidTimezoneIdentifier, e.message
+        raise InvalidTimezoneIdentifier, "#{e.message} (loading #{identifier})"
       end
     end
 
@@ -241,7 +241,7 @@ def linked_timezone_identifiers
     # or the code is invalid.
     def load_country_info(code)
       info = @country_index[code]
-      raise InvalidCountryCode, 'Invalid country code' unless info
+      raise InvalidCountryCode, "Invalid country code: #{code}" unless info
       info
     end
 
diff --git a/test/tc_country.rb b/test/tc_country.rb
index faaef11b..1265c903 100644
--- a/test/tc_country.rb
+++ b/test/tc_country.rb
@@ -20,27 +20,22 @@ def test_get_valid
   end
 
   def test_get_not_exist
-    assert_raises(InvalidCountryCode) {
-      Country.get('ZZ')
-    }
+    error = assert_raises(InvalidCountryCode) { Country.get('ZZ') }
+    assert_match(/\bZZ\b/, error.message)
   end
 
   def test_get_invalid
-    assert_raises(InvalidCountryCode) {
-      Country.get('../Countries/GB')
-    }
+    error = assert_raises(InvalidCountryCode) { Country.get('../Countries/GB') }
+    assert_match(/\W\.\.\/Countries\/GB\b/, error.message)
   end
 
   def test_get_nil
-    assert_raises(InvalidCountryCode) {
-      Country.get(nil)
-    }
+    assert_raises(InvalidCountryCode) { Country.get(nil) }
   end
 
   def test_get_case
-    assert_raises(InvalidCountryCode) {
-      Country.get('gb')
-    }
+    error = assert_raises(InvalidCountryCode) { Country.get('gb') }
+    assert_match(/\bgb\b/, error.message)
   end
 
   def test_get_tainted_loaded
@@ -82,9 +77,7 @@ def test_get_tainted_and_frozen_not_previously_loaded
   end
 
   def test_new_nil
-    assert_raises(InvalidCountryCode) {
-      Country.new(nil)
-    }
+    assert_raises(InvalidCountryCode) { Country.new(nil) }
   end
 
   def test_new_arg
@@ -93,9 +86,8 @@ def test_new_arg
   end
 
   def test_new_arg_not_exist
-    assert_raises(InvalidCountryCode) {
-      Country.new('ZZ')
-    }
+    error = assert_raises(InvalidCountryCode) { Country.new('ZZ') }
+    assert_match(/\bZZ\b/, error.message)
   end
 
   def test_all_codes
diff --git a/test/tc_ruby_data_source.rb b/test/tc_ruby_data_source.rb
index 098cff20..5765e62d 100644
--- a/test/tc_ruby_data_source.rb
+++ b/test/tc_ruby_data_source.rb
@@ -25,13 +25,15 @@ def test_load_timezone_info_does_not_exist
       @data_source.load_timezone_info('Nowhere/Special')
     end
 
-    assert_match 'Nowhere/Special', error.message
+    assert_match(/\bNowhere\/Special\b/, error.message)
   end
 
   def test_load_timezone_info_invalid
-    assert_raises(InvalidTimezoneIdentifier) do
+    error = assert_raises(InvalidTimezoneIdentifier) do
       @data_source.load_timezone_info('../Definitions/UTC')
     end
+
+    assert_match(/\W\.\.\/Definitions\/UTC\b/, error.message)
   end
 
   def test_load_timezone_info_nil
@@ -45,7 +47,7 @@ def test_load_timezone_info_case
       @data_source.load_timezone_info('europe/london')
     end
 
-    assert_match 'europe/london', error.message
+    assert_match(/\beurope\/london\b/, error.message)
   end
 
   def test_load_timezone_info_plus
@@ -103,13 +105,15 @@ def test_load_country_info_not_exist
       @data_source.load_country_info('ZZ')
     end
 
-    assert_match 'ZZ', error.message
+    assert_match(/\bZZ\b/, error.message)
   end
 
   def test_load_country_info_invalid
-    assert_raises(InvalidCountryCode) do
+    error = assert_raises(InvalidCountryCode) do
       @data_source.load_country_info('../Countries/GB')
     end
+
+    assert_match(/\W\.\.\/Countries\/GB\b/, error.message)
   end
 
   def test_load_country_info_nil
@@ -119,9 +123,11 @@ def test_load_country_info_nil
   end
 
   def test_load_country_info_case
-    assert_raises(InvalidCountryCode) do
+    error = assert_raises(InvalidCountryCode) do
       @data_source.load_country_info('gb')
     end
+
+    assert_match(/\bgb\b/, error.message)
   end
 
   def test_load_country_info_tainted
diff --git a/test/tc_timezone.rb b/test/tc_timezone.rb
index dcf74c50..b4195607 100644
--- a/test/tc_timezone.rb
+++ b/test/tc_timezone.rb
@@ -205,15 +205,13 @@ def test_get_same_instance
   end
 
   def test_get_not_exist
-    error = assert_raises(InvalidTimezoneIdentifier) do
-      Timezone.get('Nowhere/Special')
-    end
-
-    assert_match 'Nowhere/Special', error.message
+    error = assert_raises(InvalidTimezoneIdentifier) { Timezone.get('Nowhere/Special') }
+    assert_match(/\bNowhere\/Special/, error.message)
   end
 
   def test_get_invalid
-    assert_raises(InvalidTimezoneIdentifier) { Timezone.get('../Definitions/UTC') }
+    error = assert_raises(InvalidTimezoneIdentifier) { Timezone.get('../Definitions/UTC') }
+    assert_match(/\W\.\.\/Definitions\/UTC\b/, error.message)
   end
 
   def test_get_nil
@@ -223,8 +221,7 @@ def test_get_nil
   def test_get_case
     Timezone.get('Europe/Prague')
     error = assert_raises(InvalidTimezoneIdentifier) { Timezone.get('Europe/prague') }
-
-    assert_match 'Europe/prague', error.message
+    assert_match(/\bEurope\/prague\b/, error.message)
   end
 
   def test_get_proxy_valid
@@ -323,7 +320,8 @@ def test_new_arg
   end
 
   def test_new_arg_not_exist
-    assert_raises(InvalidTimezoneIdentifier) { Timezone.new('Nowhere/Special') }
+    error = assert_raises(InvalidTimezoneIdentifier) { Timezone.new('Nowhere/Special') }
+    assert_match(/\bNowhere\/Special\b/, error.message)
   end
 
   def test_all
diff --git a/test/tc_timezone_proxy.rb b/test/tc_timezone_proxy.rb
index 360a9370..f3899883 100644
--- a/test/tc_timezone_proxy.rb
+++ b/test/tc_timezone_proxy.rb
@@ -3,19 +3,24 @@
 include TZInfo
 
 class TCTimezoneProxy < Minitest::Test
+  def assert_raises_invalid_timezone_identifier(identifier)
+    error = assert_raises(InvalidTimezoneIdentifier) { yield }
+    assert_match(Regexp.new('\b' + Regexp.escape(identifier) + '\b'), error.message)
+  end
+
   def test_not_exist
     proxy = TimezoneProxy.new('Nothing/Special')
     assert_equal('Nothing/Special', proxy.identifier)
-    assert_raises(InvalidTimezoneIdentifier) { proxy.now }
-    assert_raises(InvalidTimezoneIdentifier) { proxy.current_period }
-    assert_raises(InvalidTimezoneIdentifier) { proxy.current_period_and_time }
-    assert_raises(InvalidTimezoneIdentifier) { proxy.current_time_and_period }
-    assert_raises(InvalidTimezoneIdentifier) { proxy.utc_to_local(DateTime.new(2006,1,1,0,0,0)) }
-    assert_raises(InvalidTimezoneIdentifier) { proxy.local_to_utc(DateTime.new(2006,1,1,0,0,0)) }
-    assert_raises(InvalidTimezoneIdentifier) { proxy.period_for_utc(DateTime.new(2006,1,1,0,0,0)) }
-    assert_raises(InvalidTimezoneIdentifier) { proxy.period_for_local(DateTime.new(2006,1,1,0,0,0)) }
-    assert_raises(InvalidTimezoneIdentifier) { proxy.canonical_identifier }
-    assert_raises(InvalidTimezoneIdentifier) { proxy.canonical_zone }
+    assert_raises_invalid_timezone_identifier('Nothing/Special') { proxy.now }
+    assert_raises_invalid_timezone_identifier('Nothing/Special') { proxy.current_period }
+    assert_raises_invalid_timezone_identifier('Nothing/Special') { proxy.current_period_and_time }
+    assert_raises_invalid_timezone_identifier('Nothing/Special') { proxy.current_time_and_period }
+    assert_raises_invalid_timezone_identifier('Nothing/Special') { proxy.utc_to_local(DateTime.new(2006,1,1,0,0,0)) }
+    assert_raises_invalid_timezone_identifier('Nothing/Special') { proxy.local_to_utc(DateTime.new(2006,1,1,0,0,0)) }
+    assert_raises_invalid_timezone_identifier('Nothing/Special') { proxy.period_for_utc(DateTime.new(2006,1,1,0,0,0)) }
+    assert_raises_invalid_timezone_identifier('Nothing/Special') { proxy.period_for_local(DateTime.new(2006,1,1,0,0,0)) }
+    assert_raises_invalid_timezone_identifier('Nothing/Special') { proxy.canonical_identifier }
+    assert_raises_invalid_timezone_identifier('Nothing/Special') { proxy.canonical_zone }
   end
 
   def test_valid
diff --git a/test/tc_zoneinfo_data_source.rb b/test/tc_zoneinfo_data_source.rb
index 476d4fd1..8031dc36 100644
--- a/test/tc_zoneinfo_data_source.rb
+++ b/test/tc_zoneinfo_data_source.rb
@@ -362,13 +362,15 @@ def test_load_timezone_info_does_not_exist
       @data_source.load_timezone_info('Nowhere/Special')
     end
 
-    assert_match 'Nowhere/Special', error.message
+    assert_match(/\bNowhere\/Special\b/, error.message)
   end
 
   def test_load_timezone_info_invalid
-    assert_raises(InvalidTimezoneIdentifier) do
+    error = assert_raises(InvalidTimezoneIdentifier) do
       @data_source.load_timezone_info('../Definitions/Europe/London')
     end
+
+    assert_match(/\W\.\.\/Definitions\/Europe\/London\b/, error.message)
   end
 
   def test_load_timezone_info_ignored_file
@@ -376,7 +378,7 @@ def test_load_timezone_info_ignored_file
       @data_source.load_timezone_info('localtime')
     end
 
-    assert_match 'localtime', error.message
+    assert_match(/\blocaltime\b/, error.message)
   end
 
   def test_load_timezone_info_ignored_plus_version_file
@@ -393,9 +395,11 @@ def test_load_timezone_info_ignored_plus_version_file
 
       data_source = ZoneinfoDataSource.new(dir)
 
-      assert_raises(InvalidTimezoneIdentifier) do
+      error = assert_raises(InvalidTimezoneIdentifier) do
         data_source.load_timezone_info('+VERSION')
       end
+
+      assert_match(/\W\+VERSION/, error.message)
     end
   end
 
@@ -410,7 +414,7 @@ def test_load_timezone_info_case
       @data_source.load_timezone_info('europe/london')
     end
 
-    assert_match 'europe/london', error.message
+    assert_match(/\beurope\/london/, error.message)
   end
 
   def test_load_timezone_info_permission_denied
@@ -424,9 +428,11 @@ def test_load_timezone_info_permission_denied
 
       data_source = ZoneinfoDataSource.new(dir)
 
-      assert_raises(InvalidTimezoneIdentifier) do
+      error = assert_raises(InvalidTimezoneIdentifier) do
         data_source.load_timezone_info('UTC')
       end
+
+      assert_match(/\bUTC\b/, error.message)
     end
   end
 
@@ -440,9 +446,11 @@ def test_load_timezone_info_directory
 
       data_source = ZoneinfoDataSource.new(dir)
 
-      assert_raises(InvalidTimezoneIdentifier) do
+      error = assert_raises(InvalidTimezoneIdentifier) do
         data_source.load_timezone_info('Subdir')
       end
+
+      assert_match(/\bSubdir\b/, error.message)
     end
   end
 
@@ -591,7 +599,7 @@ def test_load_timezone_info_invalid_file
         data_source.load_timezone_info('Zone')
       end
 
-      assert_match 'Zone', error.message
+      assert_match(/\bZone\b/, error.message)
     end
   end
 
@@ -617,7 +625,7 @@ def test_load_timezone_info_invalid_file_2
         data_source.load_timezone_info('Zone')
       end
 
-      assert_match 'Zone', error.message
+      assert_match(/\bZone\b/, error.message)
     end
   end
 
@@ -753,15 +761,19 @@ def test_load_country_info
   end
 
   def test_load_country_info_not_exist
-    assert_raises(InvalidCountryCode) do
+    error = assert_raises(InvalidCountryCode) do
       @data_source.load_country_info('ZZ')
     end
+
+    assert_match(/\bZZ\b/, error.message)
   end
 
   def test_load_country_info_invalid
-    assert_raises(InvalidCountryCode) do
+    error = assert_raises(InvalidCountryCode) do
       @data_source.load_country_info('../Countries/GB')
     end
+
+    assert_match(/\W\.\.\/Countries\/GB\b/, error.message)
   end
 
   def test_load_country_info_nil
@@ -771,9 +783,11 @@ def test_load_country_info_nil
   end
 
   def test_load_country_info_case
-    assert_raises(InvalidCountryCode) do
+    error = assert_raises(InvalidCountryCode) do
       @data_source.load_country_info('gb')
     end
+
+    assert_match(/\bgb\b/, error.message)
   end
 
   def test_load_country_info_tainted