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

Show a diff for comparisons of arrays of one-line strings #597

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nevinera
Copy link
Contributor

@nevinera nevinera commented May 9, 2024

A long time ago, a fellow notice a regression in the behavior of diffing. Roughly speaking, expect(["foo"]).to eq(["foo", "baz"]) doesn't print a "Diff" section of output. But expect(["foo"]).to eq(["foo", "baz\n"]) does. And that seems like an odd distinction to make! To the code mines!

I had to trace this across the move from rspec-expectations into rspec-support, but it was originally introduced here, which was motivated by this issue. That seems like a reasonable goal, and we do actually have some tests enforcing it already (though they're a little entangled with the encoding specs - I can add another test specific to this purpose if you like). The effect it had on diffing arrays of strings seems incidental though, a result of the way we flatten the arrays to test them, rather than checking them individually.

So! Break that check out ahead, and make it a bit clearer - add pointless_diff?, which tests that both arguments are Strings, and that neither string is multiline; for that case, skip adding the diff like before, but drop the 'multiline' test that was skipping.

(instead of skipping them). This behavior was originally added in in
rspec-expectations commit 044b0a6, as part of
rspec/rspec-expectations#115, which was
intended to `expect("a").to eq("b")` from showing an extra diff, when
the error message already showed the whole diff (so the diff itself was
kind of unnecessary). There are tests already enforcing _that_, so we
don't need to add any, but this change retains that behavior (though
single-element single-line array comparisons do now get a diff).
@nevinera
Copy link
Contributor Author

nevinera commented May 9, 2024

Ah, and now I think I get to learn why the mono-repo is such a desirous outcome :-)

Hm, these failures are all in rspec-mocks. Only one of them looks like it legitimately intended to test the thing I'm now braeaking - the others are all just specifying failure messages, which are changing (by gaining diffs). I could update all of those specs by making them sufficiently specific regexes, but this last one I'm not sure about:

Diffs printed when arguments don't match with a non matcher object does not print a diff when multiple single-line string arguments are mismatched

That's pretty specific. Let's dive into the history - it was added as part of this PR. It does look like it reflects the intent of the code at the time, and it predates the differ. I'm going to see if I can accommodate that intent easily.

@nevinera
Copy link
Contributor Author

nevinera commented May 9, 2024

I'm going to see if I can accommodate that intent easily.

Narrator: "He could not."

It's essentially doing exactly the thing we were trying to change; diffing two arrays of two single-line strings. So - the options are (a) update the specs to reflect a new intent, (b) cancel this plan, or (c)switch plans - the inconsistency could also be resolved by just not diffing arrays of strings at all.

I favor (a), despite the extra PRs it'll involve (go, monorepo!) - the output it produces looks like these, which seems sufficiently useful to me:

     Failure/Error: d.foo("argA", "argB", "ARGTHREE", "arg2")

       #<Double "double"> received :foo with unexpected arguments
         expected: ("arg1", "arg2", "arg3", "arg4")
              got: ("argA", "argB", "ARGTHREE", "arg2")
       Diff:

       @@ -1,5 +1,5 @@
       -arg1
       +argA
       +argB
       +ARGTHREE
        arg2
       -arg3
       -arg4


    Failure/Error: d.foo("arg1", "arg2", "ARGTHREE", "arg4")

       #<Double "double"> received :foo with unexpected arguments
         expected: ("arg1", "arg2", "arg3", "arg4")
              got: ("arg1", "arg2", "ARGTHREE", "arg4")
       Diff:
       @@ -1,5 +1,5 @@
        arg1
        arg2
       -arg3
       +ARGTHREE
        arg4

If I get buy-in, I'll make a PR to update the specs on rspec-mocks to accomodate the change I have here (I'll just remove the one spec, and I'll make the others use regexes that pass on either version of rspec-support). After that releases, we can ship this one, assuming I understand your version-relationships properly :-)

@nevinera nevinera marked this pull request as draft May 10, 2024 13:04
@nevinera
Copy link
Contributor Author

nevinera commented May 17, 2024

@pirj / @JonRowe , does that plan seem appropriate?

@pirj
Copy link
Member

pirj commented May 17, 2024

I’m all for the plan a!

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

Successfully merging this pull request may close these issues.

2 participants