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

Change method dispatch behavior to that of jmock2 #395

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

Conversation

nitishr
Copy link
Contributor

@nitishr nitishr commented Oct 31, 2019

As suggested in #173

@floehopper
Copy link
Member

I thought I should explain that I haven't reviewed this PR yet, not because I don't welcome it, but given that it will probably require a major version bump, I've de-prioritised it in my head. I hope that's OK.

@nitishr
Copy link
Contributor Author

nitishr commented Nov 6, 2019

I understand, and am not surprised. Do you have any idea when you'd be up for a major version bump, though?

@floehopper
Copy link
Member

@nitishr I've rebased a copy of your branch against the latest master and pushed it up here: https://github.com/freerange/mocha/compare/change-method-dispatch-behavior-to-that-of-jmock2-without-merge-commit.

I'm definitely up for a major version release. The tricky thing about this is that I don't think it'll be possible to display deprecation warnings and so even with a major version bump, I think I might want users to be able to revert to the current behaviour by changing a configuration option. What do you think?

Ideally I'd also like to include the removal of support for some of the things I've deprecated in the current release, e.g. support for Ruby v1.8.7, and support for older versions of test-unit and minitest.

@nitishr
Copy link
Contributor Author

nitishr commented Nov 26, 2019

What if we displayed a deprecation warning for every ExpectationList that had more than one expectation? Or if any ExpectationList had more than one expectation? Haven't thought the practicality of implementation yet, but as an idea, how does that sound?

The alternative would be to provide a config switch as you've suggested. My slight hesitance for that is because it'd mean more complexity not just in the implementation but also in the documentation and tests, as we'll have to implement, document and test both the behaviors. None of those is a big deal, but I think it'd be a lot simpler (and probably better) to emit a deprecation warning as above.

What d'you think?

test_should_keep_finding_later_stub_and_so_never_satisfy_earlier_expectation
would pass even if the earlier expectation was found first, as assert_failed
would succeed for the wrong reason, i.e. due to failure of assert_equal rather
than an unsatisfied expectation
The old behavior modelled on the method dispatch behaviour of jMock v1 caused
slightly confusing expectation behaviour when stubbing after expectation
@nitishr nitishr force-pushed the change-method-dispatch-behavior-to-that-of-jmock2 branch from a56c015 to 405b67f Compare December 24, 2019 23:39
@nitishr
Copy link
Contributor Author

nitishr commented Dec 25, 2019

I've introduced a deprecation warning as I proposed above in #448.

@floehopper floehopper changed the base branch from master to main July 24, 2020 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants