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
Calls to configure internal methods will always call base class #496
Comments
Thanks for the excellent bug report and repro. 👍 |
Thanks! I tracked down where and why this is happening, however I am not sure how to proceed at this point. The issue while it is exhibiting in the calls for NSubstitute it actually is caused by the Castle.Core proxy generator. It doesn't appear that there is a manner in which the caller (NSubstitute) can override the functionallity. Below is the call stack to the exclusion of the method from the emitted proxy. This is against Castle.Core.4.3.1 Commit # 1ba6c894ea4b011b524782f2eec6e89b6245627c
Adding the below allows for the class members to be included, but it requires changing the assembly under test to do so. And it also results in a leaking of internal implementation for NSubstitute using Castle at the assembly under test.
That's as far as I got with it, hopefully this is enough for you to proceed. It may require a feature request from Castle.Core; There may be a way to do this from a MixIn; I can't tell at this point. |
@robertbissonnette Thanks for reporting the issue and the initial investigation you did! The way Castle Proxies library works is pretty simple:
Library capabilities are limited to what you would be able to do manually if you define a regular assembly and try doing the same. In this particular example, the
While the solution might work, you should also consider that the issue might be caused by the design - it's questionable whether internal members should be tested or not. Usually, best testing approaches stick to the idea that you should test the public contract only. Nevertheless, it's a separate topic 😉 |
@zvirja Thanks It looks as if you came up with the exact same results that I did. It looks like that part of the library will have to be re-worked. I do believe that the documentation should be updated for the project to state that this is a limititation with workarounds. It clearly mentions the virtual requirement but not the public/inheritance requirement. The locations I would suggest are at minimum these two |
I've raised #498 to update the documentation. Thanks for tracking down the locations to update @robertbissonnette! @tpodolak Is it worth including this case for analyzers? NS2003 detects and suggests workarounds for internal types, but I don't think there are detections for @robertbissonnette Am I correct in guessing your code under test was in an assembly that already had |
@dtchepak In the case of fixes.. Any of the following worked:
I personally would recommend adding it to the analyzers, it is a rather subtle thing that I only caught due to a null reference being thrown. The visibility from test project into subject lib gives a false representation that it is actually able to override. Referring to NS2003, there are no checks on members, only on the type itself. This is the analyzer method in question AnalyzeTypeAccessability Based on the conventions in the Analyzers it would be another use case. |
I've raised nsubstitute/NSubstitute.Analyzers#66 on the Analyzers project to cover this case. |
@dtchepak NS2003 checks type accessibility when creating a substitute, so internal member issue doesn't fit well into this category IMHO. I would probably put it into 5xxx or alternatively into 1xxx. One way or another I think it is good to have an analyzer for that. I will try to start working on that during the weekend |
Describe the bug
NSubstitute is calling base methods even during a configure overidden call when the target member is internal
To Reproduce
Expected behaviour
Calls on internal virtual methods should not result in calls to the base type.
Environment:
Additional context
The above uses xUnit 2.3.1 to perform the calls, but the scenarios can be extracted if desired.
The text was updated successfully, but these errors were encountered: