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

Range comparison takes longer than needed for sufficiently large ranges when expectation not met #577

Open
domgetter opened this issue Jun 12, 2014 · 2 comments

Comments

@domgetter
Copy link

Due to a failed expectation running a diff on the objects involved, an expectation which involves sufficiently large ranges will iterate over every element of each range for message output upon failure. This is due to line 27 of lib/rspec/expectations/fail_with.rb

Example:

expect(1..10).to eq (1..2)

will result in:

RSpec::Expectations::ExpectationNotMetError:
expected: 1..2
     got: 1..10

(compared using ==)

Diff:
@@ -1,2 +1,2 @@
-[1, 2]
+[1, 2, 3, 4, 5, 6, 7, 8, 9, 10]

As you can see, if the range is of, say a date range spanning years, or a boundary is Float::INFINITY, the diff can take a lot of unnecessary time and screen space.

Suggested fix: wrap a conditional to check if expected or actual are ranges, and specialize output to deal with the boundaries of those ranges, rather than each element.

@JonRowe
Copy link
Member

JonRowe commented Jun 13, 2014

This is because a range is enumberable, so treated like an array, we could add another special case to the diff code (like Ssruct and hash) or we could stop treating enumerables as arrays... WDYT @myronmarston

@myronmarston
Copy link
Member

we could stop treating enumerables as arrays

This. At least for diffs. If something is not an array, I think it would be confusing to represent it as one in the diff.

That said, the place where this is happening is Composable#surface_descriptions_in, which has other call sites and uses than just the differ -- and I'm not sure if we should continue treating enumerables as arrays for those uses or not. I need to think about it more.

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