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.

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

Differ disregards trailing newlines. #70

Open
phiggins opened this issue May 26, 2014 · 3 comments
Open

Differ disregards trailing newlines. #70

phiggins opened this issue May 26, 2014 · 3 comments

Comments

@phiggins
Copy link
Contributor

Differ currently disregards any trailing newlines when making a diff of two strings. This has two main consequences I can find.

  • If two strings differ and one has a trailing newline, the diff is identical to the diff of the strings without trailing newlines:
>   puts RSpec::Support::Differ.new.diff("baz:\nfoo\n", "baz:\nbar")

@@ -1,3 +1,3 @@
 baz:
-bar
+foo
 => nil 
> puts RSpec::Support::Differ.new.diff("baz:\nfoo", "baz:\nbar")

@@ -1,3 +1,3 @@
 baz:
-bar
+foo
 => nil 

This isn't necessarily a bug, but it could make the output harder to interpret.

  • If two strings differ only in one having a trailing newline, Differ returns just a newline.
    The protocol Differ follows is to either output a diff for diffable arguments, or an empty string for non-diffable arguments. Thus two single-line strings get no diff:
> RSpec::Support::Differ.new.diff("foo", "bar")
 => "" 

One multi-line string and one single-line string get a diff:

> puts RSpec::Support::Differ.new.diff("f\noo", "bar")
@@ -1,2 +1,3 @@
-bar
+f
+oo 

Identical strings get no diff:

> RSpec::Support::Differ.new.diff("foo", "foo")
 => "" 

If two strings differ only in one having a trailing newline, Differ returns a newline:

> RSpec::Support::Differ.new.diff("foo\n", "foo")
 => "\n" 

This screws up the usage of Differ in rspec-expectations because it expects the return value of Differ to follow the "empty or diff" pattern. When I attempt to compare two strings that only differ by one newline, I get output that indicates there should be a diff but without a diff:

$ cat test_spec.rb 
describe "test case" do
  it "diffs its arguments" do
    expect("foo\n").to eq("foo")
  end
end

Trimmed output:

test case
  diffs its arguments (FAILED - 1)

Failures:

  1) test case diffs its arguments
     Failure/Error: expect("foo\n").to eq("foo")

       expected: "foo"
            got: "foo\n"

       (compared using ==)

       Diff:

     # /home/pete/projects/rspec-expectations/lib/rspec/expectations/fail_with.rb:33:in `fail_with'
     # /home/pete/projects/rspec-expectations/lib/rspec/expectations/handler.rb:36:in `handle_failure'
     # /home/pete/projects/rspec-expectations/lib/rspec/expectations/handler.rb:49:in `handle_matcher'

I'm not sure how to fix this. It looks like HunkGenerator's preprocessing of strings is getting rid of newlines so I don't know if this can be fixed without mucking with that, potentially breaking other things.

FWIW, I played around with GNU diff in these scenarios and it indicates a trailing new line in diffs:

pete@balloon:/tmp$ cat foo_trailing_newline 
baz:
foo

pete@balloon:/tmp$ cat foo_no_trailing_newline 
baz:
foo
pete@balloon:/tmp$ cat bar 
baz:
bar
pete@balloon:/tmp$ diff foo_trailing_newline foo_no_trailing_newline 
3d2
< 
pete@balloon:/tmp$ diff foo_trailing_newline bar 
2,3c2
< foo
< 

---
> bar
pete@balloon:/tmp$ diff foo_no_trailing_newline bar 
2c2
< foo

---
> bar
@JonRowe
Copy link
Member

JonRowe commented May 27, 2014

So it was a conscious decision to not diff single line strings, so this behaviour probably reflects that, I'm unsure what we should do with trailing newlines, it does feel wrong to ignore them.

@ChaeOkay
Copy link

👋 Hi @JonRowe. I'm interested in investigating this issue. I'll be working with @CoralineAda! 🌸

@JonRowe
Copy link
Member

JonRowe commented Apr 20, 2016

Sure, feel free!

jeffmccune added a commit to jeffmccune/sensu-puppet that referenced this issue Jul 25, 2017
Without this patch there are no spec tests for the sensu_check JSON provider.
This is problem because a reference is needed to specify the expected behavior
of all providers.

This patch implements a pattern of stubbing out the filesystem.  All reads and
writes in the provider itself are routed through the `read_file` and
`write_json_object` methods.  The RSpec tests then use rspec-mocks to stub out
the reads and set expectations on the output.

This reference may be applied to any provider using the `flush` method.  The
setter methods in the provider for each property are expected to update state in
an instance variable, conventionally named @property_flush but named @conf in
the sensu_check provider.  The flush method is responsible for writing out
@property_flush (@conf), which we intercept and set expectations on the data
provided.

N.B. There is a bug with Rspec where the expected and actual values of
multi-line strings will not have a nice diff output if the two strings disagree
on the presence of the trailing newline.  See
rspec/rspec-support#70 for more information.  Because
of this issue in combination with the use of IO#puts in the write_output class
method, care should be taken with the examples to make sure the expected and
actual values agree on the trailing newline.  In this patch, the fixture data
for the expected output is chomp()'ed to match the string passed to
write_output().

Resolves sensu#759
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants