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

Fix multiple expectation logging #498

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dmaclach
Copy link
Contributor

Previously multiple expectations were not being logged correctly and would be missing
names. You would get: OCClassMockObject(NSString): 2 expected methods were not invoked:.

Previously multiple expectations were not being logged correctly and would be missing
names. You would get: `OCClassMockObject(NSString): 2 expected methods were not invoked:`.
@erikdoe
Copy link
Owner

erikdoe commented Oct 14, 2021

I'm not sure I understand this change. The original code does append the names of the methods, and when I run the tests from this PR against the original code they pass. In other words: the tests from this PR do not require the code changes from this PR.

In detail: the names of the methods are appended in line 531. This is at the top-level of the loop iterating over all stubs/expectations. The only possibility that this line is not reached is that the if statemenet in line 521 evaluates to true, but that happens when the stub isn't an expectation.

Under which circumstances did you see the missing expectations without a list of the names?

@dmaclach
Copy link
Contributor Author

dmaclach commented Oct 20, 2021

I honestly can't remember. At that point I was trying to extract a bunch of changes I had made into small bits I could submit for review. I may have gone overboard here. Apologies for not doing a better job catching the error case better in the test. I can only assume I had a case I had found, wrote what I thought was a correct fix and then wrote a test but didn't verify that the test actually represented the case I was running into. Perhaps worthwhile keeping the test (because I believe it tests an untested branch) but dropping the change to OCMockObject?

@erikdoe
Copy link
Owner

erikdoe commented Oct 26, 2021

Sounds like a plan. I will merge the test only.

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