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

Incorrect output with a reused argument matcher #1287

Closed
pirj opened this issue Feb 3, 2021 · 2 comments
Closed

Incorrect output with a reused argument matcher #1287

pirj opened this issue Feb 3, 2021 · 2 comments

Comments

@pirj
Copy link
Member

pirj commented Feb 3, 2021

Subject of the issue

RSpec.describe 'A', :aggregate_failures do
  specify do
    matcher = contain_exactly(eq(1))

    expect([2]).to matcher
    expect([3]).to matcher
  end
end

The second failure message seems to have traces from the first evaluation:

            the extra elements were:        [2]

It appears that with_matchers_cloned (lib/rspec/matchers/composable.rb) doesn't cover this case.

Adding .clone or .dup to .to matcher solves the issue.

Your environment

  • Ruby version: 2.7.1
  • RSpec 3.10
    • rspec-core 3.10.1
    • rspec-expectations 3.10.1

Expected behavior

  1) A is expected to contain exactly (eq 1)
     Got 2 failures:

     1.1) Failure/Error: expect([2]).to matcher.dup

            expected collection contained:  [(eq 1)]
            actual collection contained:    [2]
            the missing elements were:      [(eq 1)]
            the extra elements were:        [2]
          # ./spec/a_spec.rb:5:in `block (2 levels) in <top (required)>'

     1.2) Failure/Error: expect([3]).to matcher.dup

            expected collection contained:  [(eq 1)]
            actual collection contained:    [3]
            the missing elements were:      [(eq 1)]
            the extra elements were:        [3]
          # ./spec/a_spec.rb:6:in `block (2 levels) in <top (required)>'

Actual behavior

  1) A is expected to contain exactly (eq 1)
     Got 2 failures:

     1.1) Failure/Error: expect([2]).to matcher

            expected collection contained:  [(eq 1)]
            actual collection contained:    [2]
            the missing elements were:      [(eq 1)]
            the extra elements were:        [2]
          # ./spec/a_spec.rb:5:in `block (2 levels) in <top (required)>'

     1.2) Failure/Error: expect([3]).to matcher

            expected collection contained:  [(eq 1)]
            actual collection contained:    [3]
            the missing elements were:      [(eq 1)]
            the extra elements were:        [2]
          # ./spec/a_spec.rb:6:in `block (2 levels) in <top (required)>'
pirj added a commit to rubocop/rspec-style-guide that referenced this issue Feb 3, 2021
Reusage may have undesired side effects.
See rspec/rspec-expectations#1287

https://github.com/rspec/rspec-expectations/blob/bd10f0cf3970932781efcd09b5e427877d16a6c2/lib/rspec/matchers/composable.rb#L113

    # Historically, a single matcher instance was only checked
    # against a single value. Given that the matcher was only
    # used once, it's been common to memoize some intermediate
    # calculation that is derived from the `actual` value in
    # order to reuse that intermediate result in the failure
    # message.
    #
    # This can cause a problem when using such a matcher as an
    # argument to another matcher in a composed matcher expression,
    # since the matcher instance may be checked against multiple
    # values and produce invalid results due to the memoization.
    #
    # To deal with this, we clone any matchers in `expected` via
    # this method when using `values_match?`, so that any memoization
    # does not "leak" between checks.

rspec/rspec-expectations#518
@JonRowe
Copy link
Member

JonRowe commented Feb 4, 2021

Some output must be cached between runs, we are supposed to be allowing this use case.

bclayman-sq added a commit to bclayman-sq/rspec-expectations that referenced this issue Oct 12, 2021
This addresses issue rspec#1287.

Previously, matchers retained state after being used as an argument
to RSpec::Expectations::ExpectationTarget#to.  This caused
incorrect results when reusing a matcher in subsequent
expectations.

This PR addresses the issue by reinitializing the matcher's
state when the matcher is reused.

Co-authored by: Rai Feren <rferen@squareup.com>
bclayman-sq added a commit to bclayman-sq/rspec-expectations that referenced this issue Oct 12, 2021
This addresses issue rspec#1287.

Previously, matchers retained state after being used as an argument
to RSpec::Expectations::ExpectationTarget#to.  This caused
incorrect results when reusing a matcher in subsequent
expectations.

This PR addresses the issue by reinitializing the matcher's
state when the matcher is reused.

Co-authored by: Rai Feren <rferen@squareup.com>
bclayman-sq added a commit to bclayman-sq/rspec-expectations that referenced this issue Oct 12, 2021
This addresses issue rspec#1287.

Previously, matchers retained state after being used as an argument
to RSpec::Expectations::ExpectationTarget#to.  This caused
incorrect results when reusing a matcher in subsequent
expectations.

This PR addresses the issue by reinitializing the matcher's
state when the matcher is reused.

Co-authored by: Rai Feren <rferen@squareup.com>
bclayman-sq added a commit to bclayman-sq/rspec-expectations that referenced this issue Oct 12, 2021
This addresses issue rspec#1287.

Previously, matchers retained state after being used as an argument
to RSpec::Expectations::ExpectationTarget#to.  This caused
incorrect results when reusing a matcher in subsequent
expectations.

This PR addresses the issue by reinitializing the matcher's
state when the matcher is reused.

Co-authored by: Rai Feren <rferen@squareup.com>
bclayman-sq added a commit to bclayman-sq/rspec-expectations that referenced this issue Oct 12, 2021
This addresses issue rspec#1287.

Previously, matchers retained state after being used as an argument
to RSpec::Expectations::ExpectationTarget#to.  This caused
incorrect results when reusing a matcher in subsequent
expectations.

This PR addresses the issue by reinitializing the matcher's
state when the matcher is reused.

Co-authored by: Rai Feren <rferen@squareup.com>
bclayman-sq added a commit to bclayman-sq/rspec-expectations that referenced this issue Oct 13, 2021
This addresses issue rspec#1287.

Previously, matchers retained state after being used as an argument
to RSpec::Expectations::ExpectationTarget#to.  This caused
incorrect results when reusing a matcher in subsequent
expectations.

This PR addresses the issue by reinitializing the matcher's
state when the matcher is reused.

Co-authored by: Rai Feren <rferen@squareup.com>
bclayman-sq added a commit to bclayman-sq/rspec-expectations that referenced this issue Oct 15, 2021
This addresses issue rspec#1287.

Previously, matchers retained state after being used as an argument
to RSpec::Expectations::ExpectationTarget#to.  This caused
incorrect results when reusing a matcher in subsequent
expectations.

This PR addresses the issue by reinitializing the matcher's
state when the matcher is reused.

Co-authored by: Rai Feren <rferen@squareup.com>
bclayman-sq added a commit to bclayman-sq/rspec-expectations that referenced this issue Apr 25, 2022
This addresses issue rspec#1287.

Previously, matchers retained state after being used as an argument
to RSpec::Expectations::ExpectationTarget#to.  This caused
incorrect results when reusing a matcher in subsequent
expectations.

This PR addresses the issue by overriding ContainExactly#matches?,
reinitializing its internal state when the matcher is reused.

Co-authored by: Rai Feren <rferen@squareup.com>
bclayman-sq added a commit to bclayman-sq/rspec-expectations that referenced this issue Apr 26, 2022
This addresses issue rspec#1287.

Previously, matchers retained state after being used as an argument
to RSpec::Expectations::ExpectationTarget#to.  This caused
incorrect results when reusing a matcher in subsequent
expectations.

This PR addresses the issue by overriding ContainExactly#matches?,
reinitializing its internal state when the matcher is reused.

Co-authored by: Rai Feren <rferen@squareup.com>
@pirj
Copy link
Member Author

pirj commented Apr 26, 2022

Fixed in #1326

@pirj pirj closed this as completed Apr 26, 2022
pirj pushed a commit that referenced this issue Nov 1, 2022
This addresses issue #1287.

Previously, matchers retained state after being used as an argument
to RSpec::Expectations::ExpectationTarget#to.  This caused
incorrect results when reusing a matcher in subsequent
expectations.

This PR addresses the issue by overriding ContainExactly#matches?,
reinitializing its internal state when the matcher is reused.

Co-authored by: Rai Feren <rferen@squareup.com>
pirj pushed a commit that referenced this issue Nov 1, 2022
This addresses issue #1287.

Previously, matchers retained state after being used as an argument
to RSpec::Expectations::ExpectationTarget#to.  This caused
incorrect results when reusing a matcher in subsequent
expectations.

This PR addresses the issue by overriding ContainExactly#matches?,
reinitializing its internal state when the matcher is reused.

Co-authored by: Rai Feren <rferen@squareup.com>
JonRowe pushed a commit that referenced this issue Nov 10, 2022
This addresses issue #1287.

Previously, matchers retained state after being used as an argument
to RSpec::Expectations::ExpectationTarget#to.  This caused
incorrect results when reusing a matcher in subsequent
expectations.

This PR addresses the issue by overriding ContainExactly#matches?,
reinitializing its internal state when the matcher is reused.

Co-authored by: Rai Feren <rferen@squareup.com>
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

2 participants