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 PropertyType.Alias to virtual #5885

Merged
merged 1 commit into from
Jul 16, 2019
Merged

Change PropertyType.Alias to virtual #5885

merged 1 commit into from
Jul 16, 2019

Conversation

hifi-phil
Copy link
Contributor

When unit testing PropertyType.Alias needs to be virtual to make it mockable. The underlying code on the set triggers IOC logic and makes it hard to test anything that uses Property Types.

This may have some security implications so I'll understand if you leave this as is....

When unit testing PropertyType.Alias needs to be virtual to make it mockable. The underlying code on the set triggers IOC logic and makes it hard to test anything that uses Property Types.
@Shazwazza
Copy link
Contributor

it is get/set though so can you not just set the alias to what you want?

@hifi-phil
Copy link
Contributor Author

yes, I could, but does that not overly complicate testing? it spoils the flow of the developer doing the testing, giving them errors that are not expected and forcing them to use creative coding to get around a problem that can be easily fixed by just making it easier to mock

@Shazwazza
Copy link
Contributor

sure, just asking the questions that need to be asked ;) since none of the other properties are virtual though, does it mean inevitably they will all need to be virtual? Ideally it's an interface but that will be a breaking change so can't do that now. I'm fine with making the alias virtual but what about everything else with regards to testing? Making everything virtual starts to get a little ugly.

@hifi-phil
Copy link
Contributor Author

I asked the same questions myself when I was thinking about submitting the PR. I agree it would be much better as an Interface but as you said that would be a breaking change. Also setting all properties as virtual would get a little ugly, but I don't think that will be the case unless the underlying functionality is changed significantly and that might mean that it can be changed to an interface? I tried to make a surgical change to fix the immediate problem and make the developer experience better...

@Shazwazza
Copy link
Contributor

All good, if you are happy with just the alias being virtual, i'll merge it in. I'd assume that's the main thing needed for testing in most cases.

@Shazwazza Shazwazza merged commit f6413af into umbraco:v8/dev Jul 16, 2019
@hifi-phil
Copy link
Contributor Author

Thanks, Shannon. Yes, this is the property used most in testing, it will make a big difference...

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.

4 participants