Refactor/cck testing (#1791)

* When there is no error, return nil instead of empty array
Apportion an undiagnosable error when it occurs

* Ensure both `nil` and `[]` are cleaned up in test response
Avoid void context of items not being returned when erroneous

* Remove redundant part of keyschecker

* Refactoring of bloated class now complete

* Add changelog

* Fix situation for blank keys needing compacting
Fix situation where extra keys are found but they are :ci inside a meta message

* Fix up tests to deal with nil response vs empty response

Amend one test to signify we only return 1 error now not 2

* Rubocop fix

Author: luke-hill

CHANGELOG.md

@@ -11,6 +11,7 @@ Please visit [cucumber/CONTRIBUTING.md](https://github.com/cucumber/cucumber/blo
 ## [Unreleased]
 ### Changed
 - Updated `cucumber-compatibility-kit` to v19
+- Optimised `compatibility` tests (That use the CCK), so that tests run slightly more optimal (Creating less empty arrays)
 
 ### Fixed
 - Fixed an issue where the html-formatter wasn't respecting the new structure for `StackTrace` cucumber messages ([#1790](https://github.com/cucumber/cucumber-ruby/pull/1790) [luke-hill](https://github.com/luke-hill))

compatibility/spec/cck/keys_checker_spec.rb

@@ -6,39 +6,40 @@
 
 describe CCK::KeysChecker do
   describe '#compare' do
-    let(:expected_values) { Cucumber::Messages::Attachment.new(url: 'https://foo.com', file_name: 'file.extension') }
-    let(:erroneous_values) { Cucumber::Messages::Attachment.new(source: '1', test_step_id: '123') }
-    let(:wrong_values) { Cucumber::Messages::Attachment.new(url: 'https://otherfoo.com', file_name: 'file.other') }
+    let(:expected_kvps) { Cucumber::Messages::Attachment.new(url: 'https://foo.com', file_name: 'file.extension', test_step_id: 123_456) }
+    let(:missing_kvps) { Cucumber::Messages::Attachment.new(url: 'https://foo.com') }
+    let(:extra_kvps) { Cucumber::Messages::Attachment.new(url: 'https://foo.com', file_name: 'file.extension', test_step_id: 123_456, source: '1') }
+    let(:missing_and_extra_kvps) { Cucumber::Messages::Attachment.new(file_name: 'file.extension', test_step_id: 123_456, test_run_started_id: 123_456) }
+    let(:wrong_values) { Cucumber::Messages::Attachment.new(url: 'https://otherfoo.com', file_name: 'file.other', test_step_id: 456_789) }
 
     it 'finds missing keys' do
-      expect(described_class.compare(erroneous_values, expected_values)).to include(
-        'Missing keys in message Cucumber::Messages::Attachment: [:file_name, :url]'
+      expect(described_class.compare(missing_kvps, expected_kvps)).to eq(
+        'Missing keys in message Cucumber::Messages::Attachment: [:file_name, :test_step_id]'
       )
     end
 
     it 'finds extra keys' do
-      expect(described_class.compare(erroneous_values, expected_values)).to include(
-        'Detected extra keys in message Cucumber::Messages::Attachment: [:source, :test_step_id]'
+      expect(described_class.compare(extra_kvps, expected_kvps)).to eq(
+        'Detected extra keys in message Cucumber::Messages::Attachment: [:source]'
       )
     end
 
-    it 'finds extra and missing keys' do
-      expect(described_class.compare(erroneous_values, expected_values)).to contain_exactly(
-        'Missing keys in message Cucumber::Messages::Attachment: [:file_name, :url]',
-        'Detected extra keys in message Cucumber::Messages::Attachment: [:source, :test_step_id]'
+    it 'finds the extra keys first' do
+      expect(described_class.compare(missing_and_extra_kvps, expected_kvps)).to eq(
+        'Detected extra keys in message Cucumber::Messages::Attachment: [:test_run_started_id]'
       )
     end
 
     it 'does not care about the values' do
-      expect(described_class.compare(expected_values, wrong_values)).to be_empty
+      expect(described_class.compare(expected_kvps, wrong_values)).to be_nil
     end
 
     context 'when default values are omitted' do
       let(:default_set) { Cucumber::Messages::Duration.new(seconds: 0, nanos: 12) }
       let(:default_not_set) { Cucumber::Messages::Duration.new(nanos: 12) }
 
       it 'does not raise an exception' do
-        expect(described_class.compare(default_set, default_not_set)).to be_empty
+        expect(described_class.compare(default_set, default_not_set)).to be_nil
       end
     end
 
@@ -49,7 +50,7 @@
         detected = Cucumber::Messages::Meta.new(ci: Cucumber::Messages::Ci.new(name: 'Some CI'))
         expected = Cucumber::Messages::Meta.new
 
-        expect(described_class.compare(detected, expected)).to be_empty
+        expect(described_class.compare(detected, expected)).to be_nil
       end
     end
 

compatibility/support/cck/keys_checker.rb

@@ -14,17 +14,20 @@ def initialize(detected, expected)
     end
 
     def compare
-      return [] if identical_keys?
+      return if identical_keys?
+      return "Detected extra keys in message #{message_name}: #{extra_keys}" if extra_keys.any?
 
-      errors << "Detected extra keys in message #{message_name}: #{extra_keys}" if extra_keys.any?
-      errors << "Missing keys in message #{message_name}: #{missing_keys}" if missing_keys.any?
-      errors
+      "Missing keys in message #{message_name}: #{missing_keys}" if missing_keys.any?
     rescue StandardError => e
       ["Unexpected error: #{e.message}"]
     end
 
     private
 
+    def identical_keys?
+      detected_keys == expected_keys
+    end
+
     def detected_keys
       @detected_keys ||= ordered_uniq_hash_keys(detected)
     end
@@ -33,32 +36,24 @@ def expected_keys
       @expected_keys ||= ordered_uniq_hash_keys(expected)
     end
 
-    def identical_keys?
-      detected_keys == expected_keys
-    end
-
-    def missing_keys
-      (expected_keys - detected_keys).reject { |key| meta_message? && key == :ci }
+    def ordered_uniq_hash_keys(object)
+      object.to_h(reject_nil_values: true).keys.sort
     end
 
     def extra_keys
       (detected_keys - expected_keys).reject { |key| meta_message? && key == :ci }
     end
 
+    def missing_keys
+      (expected_keys - detected_keys).reject { |key| meta_message? && key == :ci }
+    end
+
     def meta_message?
       detected.instance_of?(Cucumber::Messages::Meta)
     end
 
     def message_name
       detected.class.name
     end
-
-    def ordered_uniq_hash_keys(object)
-      object.to_h(reject_nil_values: true).keys.sort
-    end
-
-    def errors
-      @errors ||= []
-    end
   end
 end

compatibility/support/cck/messages_comparator.rb

@@ -12,7 +12,7 @@ def initialize(detected, expected)
     end
 
     def errors
-      all_errors.flatten
+      all_errors.compact
     end
 
     private
@@ -51,7 +51,6 @@ def compare_message(detected, expected)
       return if ignorable?(detected)
       return if incomparable?(detected)
 
-      # TODO: This needs refactoring as it's becoming rather large and bloated
       all_errors << CCK::KeysChecker.compare(detected, expected)
       compare_sub_messages(detected, expected)
     end