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

Slow sequential And #1331

Open
Lukom opened this issue Oct 27, 2021 · 3 comments · May be fixed by #1334
Open

Slow sequential And #1331

Lukom opened this issue Oct 27, 2021 · 3 comments · May be fixed by #1334

Comments

@Lukom
Copy link

Lukom commented Oct 27, 2021

Subject of the issue

It appears that if And compounds are sequential then tests are running for a long time. Eg.:

expect('qwe').
  to include('asd').
    and(include('asd')).
    and(include('asd')).
    and(include('asd')).
    and(include('asd')).
    and(include('asd')).
    and(include('asd')).
    and(include('asd')).
    and(include('asd')).
    and(include('asd')).
    and(include('asd')).
    and(include('asd')).
    and(include('asd')).
    and(include('asd'))

this is running on my computer for 15 seconds, and each additional condition doubles the previous duration.

It's not a problem with nested and:

expect('qwe').
  to include('asd').
    and include('asd').
      and include('asd').
        and include('asd').
          and include('asd').
            and include('asd').
              and include('asd').
                and include('asd').
                  and include('asd').
                    and include('asd').
                      and include('asd').
                        and include('asd').
                          and include('asd').
                            and include('asd')

This is instant. But I prefer the previous sequential approach as it doesn't increase nesting level.

Your environment

  • Ruby version: 2.6.3
  • rspec-expectations version: 3.10.1
@pirj
Copy link
Member

pirj commented Oct 30, 2021

Thanks for reporting, this looks interesting.

Would you like to debug and pinpoint the issue? I'd start somewhere here. A pull request is welcome, too.

Side note:

it doesn't increase nesting level

It might be editor-specific. I never had problems in mine increasing indentation level.

Wondering if users of the other style https://rubystyle.guide/#consistent-multi-line-chains are affected by the growing indentation problem.

@JonRowe
Copy link
Member

JonRowe commented Nov 2, 2021

These are both (or should be) sequential in our terminology (how your editor/formatter formats it is not the issue we'd format both of those the same), its just that the ruby precedence changes and one chains of the and matcher, and one chains off the include matcher, why that makes a difference I don't know yet.

@genehsu genehsu linked a pull request Nov 18, 2021 that will close this issue
@genehsu
Copy link

genehsu commented Nov 18, 2021

I was able to replicate the problem with both expressions. I believe they both hit the underlying problem, but ruby may compile the expressions differently.

When expressed the first way, the match expression is compiled like this (AND (AND (AND X X) X) X).

#<RSpec::Matchers::BuiltIn::Compound::And:0x000056077b68bd88
  @matcher_1=#<RSpec::Matchers::BuiltIn::Compound::And:0x000056077b68be00
    @matcher_1=#<RSpec::Matchers::BuiltIn::Include:0x000056077b68bef0 @expecteds=[3]>, 
    @matcher_2=#<RSpec::Matchers::BuiltIn::Include:0x000056077b68bef0 @expecteds=[3]>>,
  @matcher_2=#<RSpec::Matchers::BuiltIn::Include:0x000056077b68bef0 @expecteds=[3]>>

When expressed the second way, the match expression is compiled like this (AND X (AND X (AND X X))).

#<RSpec::Matchers::BuiltIn::Compound::And:0x000056077b68b978
  @matcher_1=#<RSpec::Matchers::BuiltIn::Include:0x000056077b68bef0 @expecteds=[3]>,
  @matcher_2=#<RSpec::Matchers::BuiltIn::Compound::And:0x000056077b68b9a0
    @matcher_1=#<RSpec::Matchers::BuiltIn::Include:0x000056077b68bef0 @expecteds=[3]>,
    @matcher_2=#<RSpec::Matchers::BuiltIn::Include:0x000056077b68bef0 @expecteds=[3]>>>

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 a pull request may close this issue.

4 participants