Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable logging non-string - fix #1410 #1412

Merged
merged 4 commits into from
Apr 28, 2020
Merged

Enable logging non-string - fix #1410 #1412

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ec18a5f
Enable logging non-string - fix #1410
vincent-psarga Apr 27, 2020
731c6f2
Remove unused error
vincent-psarga Apr 27, 2020
f609fba
Enable logging multiple objects in a single call (like puts)
vincent-psarga Apr 27, 2020
0db705c
Also show kwargs usse with log in tests
vincent-psarga Apr 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ Please visit [cucumber/CONTRIBUTING.md](https://github.com/cucumber/cucumber/blo

### Changed

* N/A
* `log` method can now be called with non-string objects and will run `.to_s` on them. [#1410](https://github.com/cucumber/cucumber-ruby/issues/1410)

### Removed

Expand Down
3 changes: 1 addition & 2 deletions lib/cucumber/glue/proto_world.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,7 @@ def embed(file, mime_type, _label = 'Screenshot')
end

def log(message)
raise Cucumber::LogTypeInvalid unless message.is_a?(String)
attach(message.dup, 'text/x.cucumber.log+plain')
attach(message.to_s.dup, 'text/x.cucumber.log+plain')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the dup?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise we log the object itself and it will display its value at display time, not at log time (there a test showing that and it fails without the dup)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why we used to do message.dup, but I don't think we need message.to_s.dup. I think message.to_s is sufficient.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Me, a reader, thought this was because the string would be used in a mutable context.)

Copy link
Contributor Author

@vincent-psarga vincent-psarga Apr 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I though the to_s would be enough too. In fact it's enough for non-string objects (obviously :D ) but when using a String we need the dup.

An example from the tests:

        describe 'when modifying the printed variable after the call to puts' 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')
              puts foo
              foo.upcase!
              puts foo
            end
          end

This test fails without the dup (we output A\nA instead of a\nA). I suspect that String.to_s do not return a duplicate but imply itself (also note, the test still uses puts instead of log, but that'll be removed once we removed the override of puts by log)

And @olleolleolle you're not wrong, it's just that the string might be mutated between the moment it I logged and the moment we actually display it.

end

def attach(file, media_type)
Expand Down
19 changes: 19 additions & 0 deletions spec/cucumber/glue/proto_world_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,25 @@ module Glue
end
end

describe 'when logging an object' do
define_feature <<-FEATURE
Feature: Banana party

Scenario: Monkey eats banana
When an object is logged
FEATURE

define_steps do
When('an object is logged') do
log(a: 1, b: 2, c: 3)
end
end

it 'attached the styring version on the object' do
expect(@out.string).to include '{:a=>1, :b=>2, :c=>3}'
end
end

describe 'when modifying the printed variable after the call to log' do
define_feature <<-FEATURE
Feature: Banana party
Expand Down
5 changes: 3 additions & 2 deletions spec/cucumber/glue/step_definition_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,11 +191,12 @@ def step_match(text)
run_step 'Loud'
end

it 'raises an exception if the message is not a String' do
it 'calls `to_s` if the message is not a String' do
expect(user_interface).to receive(:attach).with('["Not", 1, "string"]', 'text/x.cucumber.log+plain')
dsl.Given(/Loud/) do
log ['Not', 1, 'string']
end
expect { run_step 'Loud' }.to raise_exception(Cucumber::LogTypeInvalid)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Cucumber::LogTypeInvalid still used? Should it be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed

run_step 'Loud'
end
end

Expand Down