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

Expectation with never cardinality should display deprecation warning #681

Conversation

floehopper
Copy link
Member

Currently when an invocation matches an expectation which does not allow invocations (i.e. Expectation#never has been called on it), but the invocation also matches another expectation which does allow invocations, then the test does not fail with an unexpected invocation error.

In #679 I'm planning to change this behaviour so the test fails fast with an unexpected invocation error. This PR means we display a deprecation warning instead to give users a chance to update their code before the actual change is made.

@ducmtran
Copy link

we found several failing tests, here's one

  def test_stub_with
    Klass.stubs(:foo).with { |_| p 'inside stub' }
    Klass.foo
  end

on mocha main branch, it only produces one print statement

# Running:

"inside stub"
"inside stub"
"inside stub"
.

i couldn't find the time to debug all the failing cases so there might be other issues - we are pretty busy with other work right now so responses might be delayed, but thanks a lot for diving into this!

cc @zenspider

I'm about to make some changes in this area and this will make it easier
to see what's going on.
I'm planning to use this in a subsequent commit.
I'm planning to use this in a subsequent commit.
I'm planning to use this in a subsequent commit.
floehopper added a commit that referenced this pull request Nov 13, 2024
In the previous commit, the changes meant that a block provided to
`Expectation#with` was being invoked three times instead of once. The
original behaviour was not explicitly documented and there is code in
the wild [1] that relies on `Expectation#with` only being called once
per invocation.

I'm not convinced that test code should be relying on this behaviour,
but from a performance point-of-view, it probably makes sense to avoid
calling the matching methods on `ExpectationList` multiple times
unnecessarily. This commit changes the code in `Mock#handle_method_call`
to avoid unnecessary calls to these methods.

I've created #682 to suggest improving the docs for `Expectation#with`
when it takes a block argument to address the ambiguity that has become
apparent.

[1]: #681 (comment)
@floehopper floehopper force-pushed the expectation-with-never-cardinality-should-display-deprecation-warning branch from 094ea8f to 065fe3d Compare November 13, 2024 14:05
floehopper added a commit that referenced this pull request Nov 13, 2024
In the previous commit, the changes meant that a block provided to
`Expectation#with` was being invoked three times instead of once. The
original behaviour was not explicitly documented and there is code in
the wild [1] that relies on `Expectation#with` only being called once
per invocation.

I'm not convinced that test code should be relying on this behaviour,
but from a performance point-of-view, it probably makes sense to avoid
calling the matching methods on `ExpectationList` multiple times
unnecessarily. This commit changes the code in `Mock#handle_method_call`
to avoid unnecessary calls to these methods.

I've created #682 to suggest improving the docs for `Expectation#with`
when it takes a block argument to address the ambiguity that has become
apparent.

[1]: #681 (comment)
@floehopper
Copy link
Member Author

@ducmtran

Thanks for running your test suite against the branch with the deprecation warnings. I assume that the "failure" in the the example test is because in reality you have tests relying on Expectation#with only being called once. Is that correct? i.e. they will fail if it's called multiple times.

 def test_stub_with
    Klass.stubs(:foo).with { |_| p 'inside stub' }
    Klass.foo
  end

If so, while this was never documented behaviour to be relied upon, I've changed the code to avoid multiple invocations of the matcher block. 😓

Could you try running your test suite against the updated branch when you have time? 🙏

@ducmtran
Copy link

ducmtran commented Nov 15, 2024

  def test_mocha_never_warning
    Klass.stubs(:foo)
    Klass.expects(:foo).never
    Klass.foo
  end

hi, i tried the latest commit again - the above is the original issue we reported and the latest commit is failing it instead of giving a deprecation warning 😅

cc @zenspider

@floehopper
Copy link
Member Author

@ducmtran

Hmm. That's very odd. I have an acceptance test which is pretty much exactly that test and it is passing in the branch. To double-check, I just ran the test below and got the expected output, i.e. the test passed but a deprecation warning was reported.

Can you double-check that you were using the correct branch? i.e. expectation-with-never-cardinality-should-display-deprecation-warning (not expectation-with-never-cardinality-should-fail-fast-if-invoked) and that it includes the latest commit, i.e. 73c4ea6.

# klass_test.rb
require "bundler/inline"

gemfile do
  source "https://rubygems.org"
  gem "minitest"
  gem "mocha", git: "https://github.com/freerange/mocha.git", branch: "expectation-with-never-cardinality-should-display-deprecation-warning"
end

require "minitest/autorun"
require "mocha/minitest"

class TestKlass < Minitest::Test
  class Klass
    def self.foo; end
  end

  def test_foo_never_called
    Klass.stubs(:foo)
    Klass.expects(:foo).never
    Klass.foo
  end
end
> ruby klass_test.rb
Run options: --seed 22994

# Running:

Mocha deprecation warning at klass_test.rb:20:in `test_foo_never_called': The expectation defined at klass_test.rb:19:in `test_foo_never_called' does not allow invocations. However, TestKlass::Klass.foo() was invoked. This invocation will cause the test to fail fast in a future version of Mocha.
.

Finished in 0.001337s, 747.9431 runs/s, 747.9431 assertions/s.

1 runs, 1 assertions, 0 failures, 0 errors, 0 skips

@floehopper
Copy link
Member Author

@ducmtran Have you had a chance to look at this? Otherwise I'll probably just release it!

@ducmtran
Copy link

@floehopper apologies for the late response. Yes Github UI for "referenced this commit" confused me and i accidentally used the commit from #679 🤦 . With the correct commit, everything passed for us, with warning printed locally (we have an issue with our CI setup not printing the warning but i doubt it's relevant to this PR). Thanks!

@floehopper
Copy link
Member Author

@floehopper apologies for the late response. Yes Github UI for "referenced this commit" confused me and i accidentally used the commit from #679 🤦 . With the correct commit, everything passed for us, with warning printed locally (we have an issue with our CI setup not printing the warning but i doubt it's relevant to this PR). Thanks!

Great - thanks for letting me know. 👍

Currently when an invocation matches an expectation which does not allow
invocations (i.e. `Expectation#never` has been called on it), but the
invocation also matches another expectation which does allow
invocations, then the test does not fail with an unexpected invocation
error.

In #679 I'm planning to change this behaviour so the test fails fast
with an unexpected invocation error. This commit displays a deprecation
warning instead to give users a chance to update their code before the
actual change is made.

The new behaviour will bring the behaviour into line with what is
already the case for mocks where `Mock#stub_everything` has been called
as per this acceptance test [1] that was introduced in this commit [2].

[1]: https://github.com/freerange/mocha/blob/f2fa99197f35e2d2ce9554db63f6964057e29ce0/test/acceptance/expected_invocation_count_test.rb#L202-L214
[2]: d358377
In the previous commit, the changes meant that a block provided to
`Expectation#with` was being invoked three times instead of once. The
original behaviour was not explicitly documented and there is code in
the wild [1] that relies on `Expectation#with` only being called once
per invocation.

I'm not convinced that test code should be relying on this behaviour,
but from a performance point-of-view, it probably makes sense to avoid
calling the matching methods on `ExpectationList` multiple times
unnecessarily. This commit changes the code in `Mock#handle_method_call`
to avoid unnecessary calls to these methods.

I've created #682 to suggest improving the docs for `Expectation#with`
when it takes a block argument to address the ambiguity that has become
apparent.

[1]: #681 (comment)
@floehopper floehopper force-pushed the expectation-with-never-cardinality-should-display-deprecation-warning branch from 065fe3d to e0b6dae Compare November 24, 2024 10:25
@floehopper
Copy link
Member Author

I've tweaked the text of the deprecation warning slightly, tweaked the name of the acceptance test, and expanded the main commit note to explain that the new behaviour will bring the behaviour into line with what is already the case for mocks where Mock#stub_everything has been called as per the acceptance test below that was introduced in this commit.

def test_should_fail_fast_if_method_is_never_expected_but_is_called_once_even_if_everything_is_stubbed
test_result = run_as_test do
stub = stub_everything('stub')
stub.expects(:method).never
1.times { stub.method }
end
assert_failed(test_result)
assert_equal [
'unexpected invocation: #<Mock:stub>.method()',
'unsatisfied expectations:',
'- expected never, invoked once: #<Mock:stub>.method(any_parameters)'
], test_result.failure_message_lines
end

@floehopper floehopper merged commit 59e0ab1 into main Nov 24, 2024
16 checks passed
@floehopper floehopper deleted the expectation-with-never-cardinality-should-display-deprecation-warning branch November 24, 2024 10:27
floehopper added a commit that referenced this pull request Nov 24, 2024
Previously when an invocation matched an expectation which did not allow
invocations (i.e. `Expectation#never` had been called on it), but the
invocation also matched another expectation which did allow invocations,
then the test would not fail with an unexpected invocation error.

After #681 a deprecation warning was displayed in this scenario, but now
an unexpected invocation error is reported.

This behaviour was confusing and had led to a number of issues being
raised over the years: #44, #131, #490 & most recently #678. Previously
I had thought this was a symptom of the JMock v1 dispatch behaviour
(which _might_ still be true) and thus would be best addressed by
adopting the JMock v2 dispatch behaviour (#173). However, having
considered this specific scenario involving a "never" expectation, I've
decided to try to fix it with the changes in this commit.

Also the new behaviour will bring the behaviour into line with what is
already the case for mocks where `Mock#stub_everything` has been called
as per this acceptance test [1] that was introduced in this commit [2].

Now a test like this will fail with an unexpected invocation error:

    mock = mock('mock')
    mock.stubs(:method)
    mock.expects(:method).never
    mock.method

    unexpected invocation: #<Mock:mock>.method()
    unsatisfied expectations:
    - expected never, invoked once: #<Mock:mock>.method(any_parameters)
    satisfied expectations:
    - allowed any number of times, invoked never: #<Mock:mock>.method(any_parameters)

Closes #678. Also addresses #490, #131 & #44.

[1]: https://github.com/freerange/mocha/blob/f2fa99197f35e2d2ce9554db63f6964057e29ce0/test/acceptance/expected_invocation_count_test.rb#L202-L214
[2]: d358377
floehopper added a commit that referenced this pull request Nov 27, 2024
In #681 I added logic to display a deprecation warning about a change I
planned to make in #679. However, it turns out the logic in both was
faulty as demonstrated in this example [1] where the deprecation
warning was incorrectly displayed for the 2nd call to `Foo.create_if`:

    class Foo
      def self.create_if(condition)
        new if condition
      end
    end

    test "it creates only if condition is true" do
      Foo.expects(:new).never
      Foo.create_if(false)

      Foo.expects(:new).once
      Foo.create_if(true)
    end

This commit adds a new acceptance test to cover an example where the
logic was incorrect and updates the logic in `Mock#handle_method_call`
to fix the logic so this new test passes and none of the existing tests
fail. Unfortunately the implementation is now rather complicated and
hard to follow, but I think it will do for now. I have a couple of ideas
for ways to simplify it, but I don't have time to work on that now.

I plan to update the equivalent logic in #679 to match the new logic
implemented here.

[1]: #679 (comment)
floehopper added a commit that referenced this pull request Nov 28, 2024
c.f. 01874f1

In #681 I added logic to display a deprecation warning about a change I
planned to make in this PR. However, it turns out the logic in both was
faulty as demonstrated in this example [1] where the deprecation warning
was incorrectly displayed for the 2nd call to `Foo.create_if`:

    class Foo
      def self.create_if(condition)
        new if condition
      end
    end

    test "it creates only if condition is true" do
      Foo.expects(:new).never
      Foo.create_if(false)

      Foo.expects(:new).once
      Foo.create_if(true)
    end

This commit adds a new acceptance test to cover an example where the
logic was incorrect and updates the logic in `Mock#handle_method_call`
to fix the logic so this new test passes and none of the existing tests
fail. Unfortunately the implementation is now rather complicated and
hard to follow, but I think it will do for now. I have a couple of ideas
for ways to simplify it, but I don't have time to work on that now.

[1]: #679 (comment)
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 this pull request may close these issues.

2 participants