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

Add location recording to all macros. #431

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

Conversation

dmaclach
Copy link
Contributor

For large CI systems it can be costly to generate debug information.
Recording location data for all of the stub/expect/reject macros makes
it a lot easier to record/find failures.

Also renames location to ocm_location to avoid conflicts with objects that we are mocking that already have a location/setLocation method.

Copy link

@tzvist tzvist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take my comments with a grain of salt because I am not very familiar with this code base.

NSString *explanation = @"Did not record an invocation in OCMStub/OCMExpect/OCMReject.\n"
@"Possible causes are:\n"
@"- The receiver is not a mock object.\n"
@"- The selector conflicts with a selector implemented by OCMStubRecorder/OCMExpectationRecorder.";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give me an outline of how that would work?

OCMLocation *location = recorder.ocm_location;
NSString *explanation = @"Did not record an invocation in OCMStub/OCMExpect/OCMReject.\n"
@"Possible causes are:\n"
@"- The receiver is not a mock object.\n"
Copy link

@tzvist tzvist May 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer that we would assert that the received object isKindOfClass OCMockObject in beginStubMacro/beginExpectMacro/beginRejectMacro(and this way give a much more informative message and have a proper stack trace)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry tzvist, could you clarify which received object are you talking about? If it's what I think you are saying, we don't have that info in the begin*Macro method.

Copy link

@tzvist tzvist Jun 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking at it some more I have misunderstand the way OCMock finds the class that is stubbed, apparently my proposal would not work.

NSString *explanation = @"Did not record an invocation in OCMStub/OCMExpect/OCMReject.\n"
@"Possible causes are:\n"
@"- The receiver is not a mock object.\n"
@"- The selector conflicts with a selector implemented by OCMStubRecorder/OCMExpectationRecorder.";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give me an outline of how that would work?

@dmaclach
Copy link
Contributor Author

Removed the references to self from OCMStub, OCMExpect and OCMReject because it caused a couple of issues:

  1. Some set up code was in C functions that didn't have a self to reference
  2. It caused potential retain loop warnings/issues for calls that were made in blocks
  3. It wasn't being used by the code anyhow, and nil is a perfectly fine value for it.

For large CI systems it can be costly to generate debug information.
Recording location data for all of the stub/expect/reject macros makes
it a lot easier to record/find failures.
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