-
-
Notifications
You must be signed in to change notification settings - Fork 395
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
Speed up the ContainExactly matcher #1333
base: main
Are you sure you want to change the base?
Speed up the ContainExactly matcher #1333
Conversation
@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 |
96ba1da
to
7972abf
Compare
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 👍 |
Speed up the ContainExactly matcher by pre-emptively matching up corresponding elements in the expected and actual arrays. This addresses rspec#1006, rspec#1161. This PR is a collaboration between me and @genehsu based on a couple of our earlier PRs and discussion that resulted: 1) rspec#1325 2) rspec#1328 Co-authored-by: Gene Hsu (@genehsu)
7972abf
to
56ba3e4
Compare
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.
gene I'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! |
let(:max_runtime) { 2 } | ||
let(:actual) { Array.new(10_000) { rand(10) } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: 1000 elements should be enough to see if the algorithm update works well. Then max_runtime
can be dialed down as well, maybe back to 0.5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sure thing, just updated to 1K elements and 0.5s timeout 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
it do | ||
timeout_if_not_debugging(max_runtime) do | ||
expect { | ||
subject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subject | |
expect_actual_to_contain_exactly_expected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subject
is a substitute for expect_actual_to_contain_exactly_expected
or expect_actual_not_to_contain_exactly_expected
depending on the including example group.
Of course! This is really helpful feedback; I just:
One note on 3.: I didn't change the shared examples to use a named subject like Let me know if I've misunderstood anything here and thanks for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic job! Thank you
👋 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! |
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:
Co-authored-by: Gene Hsu gene@hsufarm.com