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.

Sign up for GitHub

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

Jump to bottom

Cache match results for compound failures #1334

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

Cache match results for compound failures #1334

wants to merge 2 commits into from

Conversation

genehsu
Copy link

@genehsu genehsu commented Nov 18, 2021 •
edited by pirj
Loading

fixes #1331

When a compound matcher fails, the failure message checks both sides of
the match to determine how to display the failure match. When multiple
compound matchers are chained, that means the match for each side of the
compound matcher may be called repeatedly for exponential growth.

To address this behavior, we can cache the match result for each side of
the compound operator each time a match is executed. We need to reset the
cache each time the match is executed because the matcher may be re-used
for a different expectation.

Added RSpec context timing before the patch:

RSpec::Matchers::BuiltIn::Compound
  when chaining many matchers together
    with long chains of compound matchers
      with a failing first matcher
        generates a failure description quickly
      with all failing matchers
        generates a failure description quickly with or
        generates a failure description quickly with and
      with a failing last matcher
        generates a failure description quickly

Finished in 2 minutes 47.7 seconds (files took 0.43342 seconds to load)
4 examples, 0 failures

Added RSpec context timing after the patch:

RSpec::Matchers::BuiltIn::Compound
  when chaining many matchers together
    with long chains of compound matchers
      with a failing last matcher
        generates a failure description quickly
      with all failing matchers
        generates a failure description quickly with and
        generates a failure description quickly with or
      with a failing first matcher
        generates a failure description quickly

Finished in 0.05168 seconds (files took 0.42805 seconds to load)
4 examples, 0 failures

pirj
pirj approved these changes Nov 18, 2021
View reviewed changes
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!
Does it make sense add timeout_if_not_debugging in the same vein you've used in #1333? The difference in running those specs is incredible - from 200s to 0.1s.
Thank you.

spec/rspec/matchers/built_in/compound_spec.rb Outdated Show resolved Hide resolved
spec/rspec/matchers/built_in/compound_spec.rb Outdated Show resolved Hide resolved
@genehsu
Copy link
Author

genehsu commented Nov 18, 2021

@pirj Style-wise, I was wondering about where the best place to reset state might be. I put the reset logic directly in the match method. I noticed that for a different issue, you add a blank reset method to match?. Should we incorporate that idea into this PR? or just concentrate on this PR.

All your suggestions seem good to me, so I'll go ahead and make them. Thanks for the look.

@genehsu
Copy link
Author

genehsu commented Nov 18, 2021

Looks great! Does it make sense add timeout_if_not_debugging in the same vein you've used in #1333?

Where do helper functions go when they're used in multiple spec files? It seems that spec/support is all shared examples and matchers. There are a few helper functions in spec_helper.rb. That seems like where timeout_if_not_debugging should go, unless you have a different suggestion?

@genehsu
Cache match results for compound failures
f72dfbc 
When a compound matcher fails, the failure message checks both sides of
the match to determine how to display the failure match.  When multiple
compound matchers are chained, that means the match for each side of the
compound matcher may be called repeatedly for exponential growth.

To address this behavior, we can cache the match result for each side of
the compound operator each time a match is executed.  We need to reset the
cache each time the match is executed because the matcher may be re-used
for a different expectation.
@genehsu genehsu force-pushed the cache-match-result-for-compound-failure-display branch from af695c0 to 74fe79e Compare November 18, 2021 21:22
@pirj
Copy link
Member

pirj commented Nov 18, 2021

few helper functions in spec_helper.rb. That seems like where timeout_if_not_debugging should go

A perfect place for it 👍

@genehsu
Refactor timeout_if_not_debugging
6359530 
Refactor `timeout_if_not_debugging` into `spec_helper` and use it in the
compound matcher spec to limit the run time when testing long chains of
compound matchers.
@genehsu genehsu force-pushed the cache-match-result-for-compound-failure-display branch from 74fe79e to 6359530 Compare November 18, 2021 22:06
@genehsu
Copy link
Author

genehsu commented Nov 18, 2021

This commit updates the timeout limit from 0.1s to 0.2s. Hopefully that's enough headroom for ruby 1.8.7.

@pirj
Copy link
Member

pirj commented Nov 19, 2021

@Lukom does this fix your case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pirj pirj pirj approved these changes

@JonRowe JonRowe Awaiting requested review from JonRowe

@benoittgt benoittgt Awaiting requested review from benoittgt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Slow sequential And
2 participants
@genehsu @pirj