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

SetupAllProperties: Defer property initialization #550

Merged
merged 1 commit into from
Dec 10, 2017

Conversation

stakx
Copy link
Contributor

@stakx stakx commented Dec 10, 2017

Let SetupAllProperties defer initialization of set up properties to the point when they are queried for the very first time. Doing this has two advantages:

  1. There is no more immediate difference in execution speed between DefaultValue.Empty and DefaultValue.Mock. (Up until now, SetupAllProperties could be orders of magnitude slower when used with the latter default value mode.)

  2. Type recursion / loops in the object graph are no longer an issue (hence the changes to the unit tests). We no longer need to keep track of already mocked types, so we can get rid of GetInitialValue and call GetDefaultValue directly.

Let `SetupAllProperties` initialize set up properties only when they
are queried for the very first time. Doing this has two advantages:

 1. There is no more immediate difference in execution speed between
    `DefaultValue.Empty` and `DefaultValue.Mock`.

 2. Type recursion / loops in the object graph are no longer an issue
    (hence the changes to the unit tests). We no longer need to keep
    track of already mocked types, so we can get rid of `GetInitial-
    Value` and call `GetDefaultValue` directly.
Assert.NotSame(mock.Object, mock.Object.Pong.Ping);

Assert.NotNull(mock.Object.Pong.Ping.Pong);
Assert.NotSame(mock.Object.Pong, mock.Object.Pong.Ping.Pong);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This unit test should guard against SetupAllProperties looping infinitely.


mock.Setups.Add(new PropertyGetterMethodCall(mock, expression, getter, () => value));
value = initialValue;
valueNotSet = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory, there's room for a race condition here. The uninitialized property could be queried simultaneously several times, or it could be queried while the setter is running, too. In practice, this might not matter much with regards to correctness, but it might cause some inefficiency because multiple initial values are produced when only one is required.

if (mockedTypesStack.Contains(getter.ReturnType))
{
// to deal with loops in the property graph
return mock.GetDefaultValue(getter, useAlternateProvider: DefaultValueProvider.Empty);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We no longer need to guard against "loops in the property graph" since SetupAllProperties only sets up the properties of the current level in the object's hierarchy. The setting up of deeper levels in the hierarchy must be triggered by querying a property.

@stakx stakx merged commit a152192 into devlooped:develop Dec 10, 2017
@stakx stakx deleted the setupallproperties branch December 10, 2017 23:06
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.

None yet

1 participant