rspec-expectations
rspec-expectations copied to clipboard
rspec
Speed up the ContainExactly matcher
Speed up the ContainExactly matcher by pre-emptively matching up corresponding elements in the expected and actual arrays.
This addresses issues #1006, #1161.
Before this PR, comparing two arrays of 50 random integers never finished. Now, comparing arrays of 10,000 random integers takes 1 second.
This PR is a collaboration between @genehsu and I based on a couple of our earlier PRs and discussion that resulted:
- https://github.com/rspec/rspec-expectations/pull/1325
- https://github.com/rspec/rspec-expectations/pull/1328
Co-authored-by: Gene Hsu [email protected]
@genehsu, Could I get your email so I can update the "co-authored-by" section of the PR description?
If this superseeds the other PR(s) can you close it/them? Will help with the review backlog which is currently sitting at 25 tabs
If this superseeds the other PR(s) can you close it/them? Will help with the review backlog which is currently sitting at 25 tabs
Yeah, I'm happy to! Mostly wanted to be sure Gene was satisfied with this outcome and I wasn't stepping on any toes, so I'll wait for them to comment before closing 👍
@bclayman-sq
Looks good. I'd suggest that 1000 random elements should be enough since we're hitting timing issues on one of the Jruby test suites.
@genehsu, Could I get your email so I can update the "co-authored-by" section of the PR description?
gene at hsufarm dot com
I'm closing my previous PR now
@bclayman-sq
Looks good. I'd suggest that 1000 random elements should be enough since we're hitting timing issues on one of the Jruby test suites.
@genehsu, Could I get your email so I can update the "co-authored-by" section of the PR description?
gene
athsufarmdotcomI'm closing my previous PR now
Ah gotcha, I ended up bumping to 2s to get around that pesky jruby one but agree in general that 1000 is probably plenty 👍
Sounds good, will update the PR description and close my earlier PR!
Great work, can we get a benchmark of the old implementation vs the new implementation added as a comment above the implementation to explain why its so complex, plus I have some wording tweaks for the tests as suggestion, especially the subject bit as we prefer named subjects where possible.
Of course! This is really helpful feedback; I just:
- Added a comment with benchmark data to justify the complexity
- Incorporated the wording tweaks
- Added named subjects where possible
One note on 3.: I didn't change the shared examples to use a named subject like expect_actual_to_contain_exactly_expected instead of subject. The specs use shared examples for both positive and negative expectations, so changing either shared example to use expect_actual_to_contain_exactly_expected or expect_actual_not_to_contain_exactly_expected wouldn't exercise the behavior we want and would cause a couple failing tests. Relying on subject in both shared examples lets me override it as appropriate in the context for a positive expectation and the context for a negative expectation.
Let me know if I've misunderstood anything here and thanks for the review!
@JonRowe @pirj Hi, hope you two are doing well! I've added a comment with benchmark data per Jon's request and wanted to see if there was anything I could do to get this PR merged in. Let me know!
(I have a couple other PRs from several months ago that I'll comment on similarly!)
@JonRowe @pirj Hi, hope you two are doing well! I've added a comment with benchmark data per Jon's request and wanted to see if there was anything I could do to get this PR merged in. Let me know!
(I have a couple other PRs from several months ago that I'll comment on similarly!)
👋 Hi @JonRowe, @pirj has approved this and I've managed to incorporate your requested changes (benchmark data, wording tweaks). I followed up a month or so ago and imagine you're slammed. I do still want to get this in, however! Would you feel OK merging this?
If it helps, I'm happy to review a pending PR or two to lighten the load on the core reviewers 👍 Appreciate your help as always!