Refactor: #attach methodology (#1797)

* A file input is either a path or a base64 IO.read object, either way we handle it by passing it to super

* Handle errors in raw byte attach (Plus potentially file open attach)

* Fix up some test inconsistencies and remove duplicate tst

* Add in new specs for diff variants including note to fix input type

* Fix order of detection of file and mime type

* Fix whitespace rubocop

* Enhance stdout response from process to be formatted in a variety of ways

* Refactor 2 steps to be more optimal using new formatter for stdout

Write a new cuke that will recurse to find the desired message and then check its internal contents for more optimal processing

* Use new steps

* Improve message formatter scenarios by using the ndjson tests and fix up attachment steps styling

* Rubocop fixes

* Add changelog

Author: luke-hill

CHANGELOG.md

@@ -9,6 +9,10 @@ This document is formatted according to the principles of [Keep A CHANGELOG](htt
 Please visit [cucumber/CONTRIBUTING.md](https://github.com/cucumber/cucumber/blob/master/CONTRIBUTING.md) for more info on how to contribute to Cucumber.
 
 ## [Unreleased]
+### Changed
+- Internal refactors to testing code to better check that message responses are as expected
+- Simplify `#attach` by better checking the different use cases (base64 vs file path)
+
 ### Fixed
 - Prevent messages (And any consuming formatters), from not handling unknown (base64), media types ([#1796](https://github.com/cucumber/cucumber-ruby/pull/1796) [luke-hill](https://github.com/luke-hill))
 

features/docs/formatters/message.feature

@@ -29,13 +29,13 @@ Feature: Message output formatter
       gherkinDocument
       pickle
       pickle
-      testCase
-      testCase
       testRunStarted
+      testCase
       testCaseStarted
       testStepStarted
       testStepFinished
       testCaseFinished
+      testCase
       testCaseStarted
       testStepStarted
       testStepFinished
@@ -51,10 +51,8 @@ Feature: Message output formatter
       """
     When I run `cucumber features/my_feature.feature --format message`
     Then output should be valid NDJSON
-    And the output should contain:
-      """
-      {"testRunFinished":{"success":false
-      """
+    And the output should contain NDJSON with key "testRunFinished"
+    And the output should contain NDJSON "testRunFinished" message with key "success" and boolean value false
 
   Scenario: it sets "testRunFinished"."success" to true if nothing failed
     Given a file named "features/my_feature.feature" with:
@@ -70,7 +68,5 @@ Feature: Message output formatter
       """
     When I run `cucumber features/my_feature.feature --format message`
     Then output should be valid NDJSON
-    And the output should contain:
-      """
-      {"testRunFinished":{"success":true
-      """
+    And the output should contain NDJSON with key "testRunFinished"
+    And the output should contain NDJSON "testRunFinished" message with key "success" and boolean value true

features/docs/writing_support_code/attachments.feature

@@ -12,43 +12,43 @@ Feature: Attachments
       """
       Feature: A screenshot feature
         Scenario:
-          Given I attach a screenshot with media type
+          Given I attach a screenshot with a media type
       """
     Given a file named "features/attaching_screenshot_without_mediatype.feature" with:
       """
       Feature: A file feature
         Scenario:
-          Given I attach a screenshot without media type
+          Given I attach a screenshot without a media type
       """
     And a file named "features/screenshot.png" with:
       """
       foo
       """
     And a file named "features/step_definitions/attaching_screenshot_steps.rb" with:
       """
-      Given /^I attach a screenshot with media type/ do
-        attach "features/screenshot.png", "image/png"
+      Given('I attach a screenshot with a media type') do
+        attach('features/screenshot.png', 'image/png')
       end
 
-      Given /^I attach a screenshot without media type/ do
-        attach "features/screenshot.png"
+      Given('I attach a screenshot without a media type') do
+        attach('features/screenshot.png')
       end
       """
 
   Scenario: Files can be attached given their path
     When I run `cucumber --format message features/attaching_screenshot_with_mediatype.feature`
     Then output should be valid NDJSON
     And the output should contain NDJSON with key "attachment"
-    And the output should contain NDJSON with key "body" and value "Zm9v"
-    And the output should contain NDJSON with key "mediaType" and value "image/png"
+    And the output should contain NDJSON "attachment" message with key "body" and value "Zm9v"
+    And the output should contain NDJSON "attachment" message with key "mediaType" and value "image/png"
 
   Scenario: Media type is inferred from the given file
     When I run `cucumber --format message features/attaching_screenshot_without_mediatype.feature`
     Then output should be valid NDJSON
     And the output should contain NDJSON with key "attachment"
-    And the output should contain NDJSON with key "body" and value "Zm9v"
-    And the output should contain NDJSON with key "mediaType" and value "image/png"
-    
+    And the output should contain NDJSON "attachment" message with key "body" and value "Zm9v"
+    And the output should contain NDJSON "attachment" message with key "mediaType" and value "image/png"
+
   Scenario: With json formatter, files can be attached given their path
     When I run `cucumber --format json features/attaching_screenshot_with_mediatype.feature`
     Then the output should contain "embeddings\":"

features/lib/step_definitions/message_steps.rb

@@ -1,22 +1,32 @@
 # frozen_string_literal: true
 
 Then('messages types should be:') do |expected_types|
-  parsed_json = command_line.stdout.split("\n").map { |line| JSON.parse(line) }
-  message_types = parsed_json.map(&:keys).flatten.compact
+  message_types = command_line.stdout(format: :messages).map(&:keys).flatten.compact
 
-  expect(expected_types.split("\n").map(&:strip)).to contain_exactly(*message_types)
+  expect(expected_types.split("\n")).to eq(message_types)
 end
 
 Then('output should be valid NDJSON') do
-  command_line.stdout.split("\n").map do |line|
-    expect { JSON.parse(line) }.not_to raise_exception
-  end
+  expect { command_line.stdout(format: :messages) }.not_to raise_error
 end
 
 Then('the output should contain NDJSON with key {string}') do |key|
-  expect(command_line.stdout).to match(/"#{key}":/)
+  expect(command_line.stdout(format: :messages)).to include(have_key(key))
 end
 
 Then('the output should contain NDJSON with key {string} and value {string}') do |key, value|
   expect(command_line.stdout).to match(/"#{key}": ?"#{value}"/)
 end
+
+Then('the output should contain NDJSON {string} message with key {string} and value {string}') do |message_name, key, value|
+  message_contents = command_line.stdout(format: :messages).detect { |msg| msg.keys == [message_name] }[message_name]
+
+  expect(message_contents).to include(key => value)
+end
+
+Then('the output should contain NDJSON {string} message with key {string} and boolean value {word}') do |message_name, key, value|
+  boolean = value == 'true'
+  message_contents = command_line.stdout(format: :messages).detect { |msg| msg.keys == [message_name] }[message_name]
+
+  expect(message_contents).to include(key => boolean)
+end

features/lib/support/command_line.rb

@@ -30,8 +30,12 @@ def stderr
     @stderr.string
   end
 
-  def stdout
-    @stdout.string
+  def stdout(format: :string)
+    case format
+    when :lines then @stdout.string.split("\n")
+    when :messages then @stdout.string.split("\n").map { |line| JSON.parse(line) }
+    else @stdout.string
+    end
   end
 
   def all_output

lib/cucumber/glue/proto_world.rb

@@ -90,12 +90,11 @@ def log(*messages)
       #   This is only needed in situations where you want to rename a PDF download e.t.c. - In most situations
       #   you should not need to pass a filename
       def attach(file, media_type = nil, filename = nil)
-        return super unless File.file?(file)
-
-        content = File.read(file, mode: 'rb')
-        media_type = MiniMime.lookup_by_filename(file)&.content_type if media_type.nil?
-
-        super(content, media_type.to_s, filename)
+        if File.file?(file)
+          media_type = MiniMime.lookup_by_filename(file)&.content_type if media_type.nil?
+          file = File.read(file, mode: 'rb')
+        end
+        super
       rescue StandardError
         super
       end

spec/cucumber/glue/proto_world_spec.rb

@@ -3,6 +3,7 @@
 require 'spec_helper'
 require 'cucumber/formatter/spec_helper'
 require 'cucumber/formatter/pretty'
+require 'cucumber/formatter/message'
 
 module Cucumber
   module Glue
@@ -32,38 +33,38 @@ module Glue
           run_defined_feature
         end
 
-        describe 'when modifying the printed variable after the call to log' do
-          define_feature <<-FEATURE
-        Feature: Banana party
+        describe 'when modifying the printed variable after the call to #log' do
+          define_feature <<~FEATURE
+            Feature: Banana party
 
-          Scenario: Monkey eats banana
-            When log is called twice for the same variable
+              Scenario: Monkey eats banana
+                When log is called twice for the same variable
           FEATURE
 
           define_steps do
-            When(/^log is called twice for the same variable$/) do
+            When('log is called twice for the same variable') do
               foo = String.new('a')
               log foo
               foo.upcase!
               log foo
             end
           end
 
-          it 'prints the variable value at the time puts was called' do
-            expect(@out.string).to include <<OUTPUT
-    When log is called twice for the same variable
-      a
-      A
-OUTPUT
+          it 'prints the variable value at the time `#log` was called' do
+            expect(@out.string).to include <<~OUTPUT
+              When log is called twice for the same variable
+                    a
+                    A
+            OUTPUT
           end
         end
 
         describe 'when logging an object' do
-          define_feature <<-FEATURE
-        Feature: Banana party
+          define_feature <<~FEATURE
+            Feature: Banana party
 
-          Scenario: Monkey eats banana
-            When an object is logged
+              Scenario: Monkey eats banana
+                When an object is logged
           FEATURE
 
           define_steps do
@@ -82,11 +83,11 @@ def object.to_s
         end
 
         describe 'when logging multiple items on one call' do
-          define_feature <<-FEATURE
-        Feature: Logging multiple entries
+          define_feature <<~FEATURE
+            Feature: Logging multiple entries
 
-          Scenario: Logging multiple entries
-            When logging multiple entries
+              Scenario: Logging multiple entries
+                When logging multiple entries
           FEATURE
 
           define_steps do
@@ -103,32 +104,6 @@ def object.to_s
             ].join("\n"))
           end
         end
-
-        describe 'when modifying the printed variable after the call to log' do
-          define_feature <<-FEATURE
-        Feature: Banana party
-
-          Scenario: Monkey eats banana
-            When puts is called twice for the same variable
-          FEATURE
-
-          define_steps do
-            When(/^puts is called twice for the same variable$/) do
-              foo = String.new('a')
-              log foo
-              foo.upcase!
-              log foo
-            end
-          end
-
-          it 'prints the variable value at the time puts was called' do
-            expect(@out.string).to include <<OUTPUT
-    When puts is called twice for the same variable
-      a
-      A
-OUTPUT
-          end
-        end
       end
 
       describe 'Handling attachments in step definitions' do
@@ -142,7 +117,7 @@ def object.to_s
           run_defined_feature
         end
 
-        describe 'when attaching data with null byte' do
+        context 'when attaching data with null byte' do
           define_feature <<~FEATURE
             Feature: Banana party
 
@@ -152,7 +127,7 @@ def object.to_s
 
           define_steps do
             When('some data is attached') do
-              attach("'\x00'attachement", 'text/x.cucumber.log+plain')
+              attach("'\x00'attachment", 'text/x.cucumber.log+plain')
             end
           end
 
@@ -161,7 +136,60 @@ def object.to_s
           end
 
           it 'properly attaches the data' do
-            expect(@out.string).to include("'\x00'attachement")
+            expect(@out.string).to include("'\x00'attachment")
+          end
+        end
+
+        context 'when attaching a image using a file path' do
+          define_feature <<~FEATURE
+            Feature: Banana party
+
+              Scenario: Monkey eats banana
+                When some data is attached
+          FEATURE
+
+          define_steps do
+            When('some data is attached') do
+              path = "#{Dir.pwd}/docs/img/cucumber-open-logo.png"
+              attach(path, 'image/png')
+            end
+          end
+
+          it 'does not report an error' do
+            expect(@out.string).not_to include('Error')
+          end
+
+          it 'properly attaches the image' do
+            pending 'This is correct currently with the pretty implementation'
+
+            expect(@out.string).to include("'\x00'attachment")
+          end
+        end
+
+        context 'when attaching a image using the input-read data' do
+          define_feature <<~FEATURE
+            Feature: Banana party
+
+              Scenario: Monkey eats banana
+                When some data is attached
+          FEATURE
+
+          define_steps do
+            When('some data is attached') do
+              path = "#{Dir.pwd}/docs/img/cucumber-open-logo.png"
+              image_data = File.read(path, mode: 'rb')
+              attach(image_data, 'base64;image/png')
+            end
+          end
+
+          it 'does not report an error' do
+            expect(@out.string).not_to include('Error')
+          end
+
+          it 'properly attaches the image' do
+            pending 'This is correct currently with the pretty implementation'
+
+            expect(@out.string).to include("'\x00'attachment")
           end
         end
       end