Merge pull request #75 from github/kpaulisse-integration-refactor

Integration refactors - exit codes, more information

Author: kpaulisse

lib/octocatalog-diff/cli.rb

@@ -116,8 +116,8 @@ def self.cli(argv = ARGV, logger = Logger.new(STDERR), opts = {})
       printer_obj = OctocatalogDiff::Cli::Printer.new(options, logger)
       printer_obj.printer(diffs, catalog_diff.from.compilation_dir, catalog_diff.to.compilation_dir)
 
-      # Return the diff object if requested (generally for testing) or otherwise return exit code
-      return diffs if opts[:RETURN_DIFFS]
+      # Return the resulting diff object if requested (generally for testing) or otherwise return exit code
+      return catalog_diff if opts[:INTEGRATION]
       diffs.any? ? EXITCODE_SUCCESS_WITH_DIFFS : EXITCODE_SUCCESS_NO_DIFFS
     end
 
@@ -169,11 +169,8 @@ def self.catalog_only(logger, options)
         puts to_catalog.to_json
       end
 
-      return EXITCODE_SUCCESS_NO_DIFFS unless options[:RETURN_DIFFS] # For integration testing
-      # :nocov:
-      dummy_catalog = OctocatalogDiff::API::V1::Catalog.new(OctocatalogDiff::Catalog.new(backend: :noop))
-      [dummy_catalog, to_catalog]
-      # :nocov:
+      return { exitcode: EXITCODE_SUCCESS_NO_DIFFS, to: to_catalog } if options[:INTEGRATION] # For integration testing
+      EXITCODE_SUCCESS_NO_DIFFS
     end
 
     # --bootstrap-then-exit command

spec/octocatalog-diff/integration/catalog_only_spec.rb

@@ -26,43 +26,36 @@
 
   it 'should compile the catalog' do
     expect(@result[:exitcode]).not_to eq(-1), OctocatalogDiff::Integration.format_exception(@result)
-    expect(@result[:exitcode]).to eq(2), "Runtime error: #{@result[:logs]}"
-  end
-
-  it 'should set the from-catalog to a no-op catalog type' do
-    pending 'catalog compilation failed' unless @result[:exitcode] == 2
-    from_catalog = @result[:diffs][0]
-    expect(from_catalog).to be_a_kind_of(OctocatalogDiff::API::V1::Catalog)
-    expect(from_catalog.builder).to eq('OctocatalogDiff::Catalog::Noop')
+    expect(@result[:exitcode]).to eq(0), "Runtime error: #{@result[:logs]}"
   end
 
   it 'should set the to-catalog to a computed catalog type' do
-    pending 'catalog compilation failed' unless @result[:exitcode] == 2
-    to_catalog = @result[:diffs][1]
+    pending 'catalog compilation failed' unless (@result[:exitcode]).zero?
+    to_catalog = @result.to
     expect(to_catalog).to be_a_kind_of(OctocatalogDiff::API::V1::Catalog)
     expect(to_catalog.builder).to eq('OctocatalogDiff::Catalog::Computed')
   end
 
   it 'should have log messages indicating catalog compilations' do
-    pending 'catalog compilation failed' unless @result[:exitcode] == 2
+    pending 'catalog compilation failed' unless (@result[:exitcode]).zero?
     logs = @result[:logs]
     expect(logs).to match(/Compiling catalog for rspec-node.github.net/)
     expect(logs).to match(/Initialized OctocatalogDiff::Catalog::Noop for from-catalog/)
     expect(logs).to match(/Initialized OctocatalogDiff::Catalog::Computed for to-catalog/)
   end
 
   it 'should produce a valid catalog' do
-    pending 'catalog compilation failed' unless @result[:exitcode] == 2
-    to_catalog = @result[:diffs][1]
+    pending 'catalog compilation failed' unless (@result[:exitcode]).zero?
+    to_catalog = @result.to
     expect(to_catalog.valid?).to eq(true)
     expect(to_catalog.to_h).to be_a_kind_of(Hash)
     expect(to_catalog.to_json).to be_a_kind_of(String)
     expect(to_catalog.error_message).to be(nil)
   end
 
   it 'should produce the expected catalog' do
-    pending 'catalog compilation failed' unless @result[:exitcode] == 2
-    to_catalog = @result[:diffs][1]
+    pending 'catalog compilation failed' unless @result.exitcode.zero?
+    to_catalog = @result.to
 
     param1 = { 'owner' => 'root', 'group' => 'root', 'mode' => '0644', 'content' => 'Testy McTesterson' }
     expect(to_catalog.resource(type: 'File', title: '/tmp/foo')['parameters']).to eq(param1)

spec/octocatalog-diff/integration/integration_helper.rb

@@ -113,39 +113,40 @@ def self.integration(options = {})
       options[:from_puppet_binary] ||= OctocatalogDiff::Spec::PUPPET_BINARY
       options[:to_puppet_binary] ||= OctocatalogDiff::Spec::PUPPET_BINARY
       options[:parallel] = false if ENV['COVERAGE']
+      options[:INTEGRATION] = true
 
       # Run octocatalog-diff CLI method. Capture stdout and stderr using 'strio'.
-      # Set options[:RETURN_DIFFS] so that the .cli method returns the JSON array
-      # of differences instead of the exit code.
       logger, logger_string = OctocatalogDiff::Spec.setup_logger
       begin
         # Capture stdout to a variable
         old_out = $stdout
         stdout_strio = StringIO.new
         $stdout = stdout_strio
 
-        # Tell OctocatalogDiff::Cli.cli to return the JSON differences and not a numeric exit code
-        # for a full catalog-diff.
-        options[:RETURN_DIFFS] = true
-
         # Run the OctocatalogDiff::Cli.cli and validate output format.
         result = OctocatalogDiff::Cli.cli(argv, logger, options)
         if result.is_a?(Fixnum)
-          return {
-            logs: logger_string.string,
-            output: stdout_strio.string,
-            exitcode: result
-          }
+          result = OpenStruct.new(exitcode: result, exception: nil, diffs: [], to: nil, from: nil)
+        elsif result.is_a?(Hash)
+          result = OpenStruct.new({ exitcode: nil, exception: nil, diffs: [], to: nil, from: nil }.merge(result))
+        elsif !result.is_a?(OpenStruct)
+          raise "Expected OpenStruct, got #{result.inspect} from OctocatalogDiff::Cli.cli!"
         end
 
-        raise "OctocatalogDiff::Cli.cli should return array, got #{result.inspect}" unless result.is_a?(Array)
+        exitcode = if result.exitcode.nil?
+          result.diffs.any? ? 2 : 0
+        else
+          result.exitcode
+        end
 
-        # Return hash
         OpenStruct.new(
           logs: logger_string.string,
+          log_messages: logger_string.string.split(/\n/).map { |x| OctocatalogDiff::Spec.strip_log_message(x) },
           output: stdout_strio.string,
-          diffs: result,
-          exitcode: result.any? ? 2 : 0,
+          diffs: result.diffs,
+          to: result.to,
+          from: result.from,
+          exitcode: exitcode,
           options: options
         )
       rescue => exc # Yes, rescue *everything*

spec/octocatalog-diff/integration/reference_validation_spec.rb

@@ -45,7 +45,7 @@ def self.catalog_contains_resource(result, type, title)
   end
 
   it 'should return the valid catalog' do
-    expect(@result.exitcode).to eq(2), OctocatalogDiff::Integration.format_exception(@result)
+    expect(@result.exitcode).to eq(0), OctocatalogDiff::Integration.format_exception(@result)
   end
 end
 
@@ -55,15 +55,15 @@ def self.catalog_contains_resource(result, type, title)
   end
 
   it 'should return the valid catalog' do
-    expect(@result.exitcode).to eq(2)
+    expect(@result.exitcode).to eq(0)
   end
 
   it 'should not raise any exceptions' do
     expect(@result.exception).to be_nil, OctocatalogDiff::Integration.format_exception(@result)
   end
 
   it 'should contain representative resources' do
-    pending 'Catalog failed' unless @result.exitcode == 2
+    pending 'Catalog failed' unless @result.exitcode.zero?
     expect(OctocatalogDiff::Spec.catalog_contains_resource(@result, 'File', '/tmp/test-main')).to eq(true)
   end
 end
@@ -75,15 +75,15 @@ def self.catalog_contains_resource(result, type, title)
     end
 
     it 'should succeed' do
-      expect(@result.exitcode).to eq(2)
+      expect(@result.exitcode).to eq(0)
     end
 
     it 'should not raise any exceptions' do
       expect(@result.exception).to be_nil, OctocatalogDiff::Integration.format_exception(@result)
     end
 
     it 'should contain representative resources' do
-      pending 'Catalog failed' unless @result.exitcode == 2
+      pending 'Catalog failed' unless @result.exitcode.zero?
       expect(OctocatalogDiff::Spec.catalog_contains_resource(@result, 'Exec', 'subscribe caller 1')).to eq(true)
       expect(OctocatalogDiff::Spec.catalog_contains_resource(@result, 'Exec', 'subscribe target')).to eq(true)
     end
@@ -179,7 +179,7 @@ def self.catalog_contains_resource(result, type, title)
     end
 
     it 'should succeed' do
-      expect(@result.exitcode).to eq(2), OctocatalogDiff::Integration.format_exception(@result)
+      expect(@result.exitcode).to eq(0), OctocatalogDiff::Integration.format_exception(@result)
     end
 
     it 'should not raise error' do
@@ -195,15 +195,15 @@ def self.catalog_contains_resource(result, type, title)
     end
 
     it 'should succeed' do
-      expect(@result.exitcode).to eq(2)
+      expect(@result.exitcode).to eq(0)
     end
 
     it 'should not raise any exceptions' do
       expect(@result.exception).to be_nil, OctocatalogDiff::Integration.format_exception(@result)
     end
 
     it 'should contain representative resources' do
-      pending 'Catalog failed' unless @result.exitcode == 2
+      pending 'Catalog failed' unless @result.exitcode.zero?
       expect(OctocatalogDiff::Spec.catalog_contains_resource(@result, 'Exec', 'before alias caller')).to eq(true)
       expect(OctocatalogDiff::Spec.catalog_contains_resource(@result, 'Exec', 'before alias target')).to eq(true)
       expect(OctocatalogDiff::Spec.catalog_contains_resource(@result, 'Exec', 'the before alias target')).to eq(true)

spec/octocatalog-diff/tests/spec_helper.rb

@@ -217,6 +217,11 @@ def self.setup_logger
     # remove the file and line locations. This takes the JSON result from catalog-diff and returns
     # a cleaned-up array of diffs.
     def self.remove_file_and_line(diff)
+      if diff.is_a?(OctocatalogDiff::API::V1::Diff)
+        result = diff.to_h.dup
+        %w(new_location old_location new_line old_line new_file old_file).each { |x| result.delete(x.to_sym) }
+        return result
+      end
       if diff.is_a?(Hash)
         result = diff.dup
         %w(new_location old_location new_line old_line new_file old_file).each { |x| result.delete(x) }
@@ -228,6 +233,13 @@ def self.remove_file_and_line(diff)
       obj.map { |x| x[0] =~ /^[\-\+]$/ ? x[0..2] : x[0..3] }
     end
 
+    # Strip off timestamps and other extraneous content from log messages so that matching
+    # of individual elements can be done via string and not regexp.
+    def self.strip_log_message(message)
+      return message unless message.strip =~ /\A\w,\s*\[[^\]]+\]\s+(\w+)\s*--\s*:(.+)/
+      "#{Regexp.last_match(1)} - #{Regexp.last_match(2).strip}"
+    end
+
     # Get the Puppet version from the Puppet binary
     def self.puppet_version
       require require_path('util/puppetversion')