-
Notifications
You must be signed in to change notification settings - Fork 253
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
Inheritance support for base classes that resides in different assemblies. #23
Comments
This could be another setting that we can expose in the "MSTestV2" section of the runsettings.. |
See here for the ask on uservoice: https://visualstudio.uservoice.com/forums/330519-team-services/suggestions/6030736-support-test-inheritance-for-base-classes-in-differ |
This looks like a relatively simple issue for a newcomer to pick up. I'd like to help out. It looks like this TODO (lline 100 of testfx/src/Adapter/MSTest.CoreAdapter/Discovery/TypeEnumerator.cs) is the place to implement it. Is MsTestSettings the right place to add the new setting? Would a good name be |
@ajryan: That would be wonderful.
Yes, that's exactly the place.
Right again. We can name the setting |
Thanks for the quick response. I don't think your suggested name is correct, a "derived type" would be an inheriting class (which already works) but we're concerned with enabling test methods found in base/parent classes. Plus it's not a type that is/isn't being enabled, it's a test method. Maybe one of these?
|
ah that could be confusing. |
Thanks to @ajryan this is now fixed and would be available in the Sprint 116 nuget release. |
After speaking with @pvlakshm, reopening this to track making this the default behavior. This would involve the following as well:
|
Removing up-for-grabs for now since this involves perf infrastructure to be setup. |
FWIW, I think it's highly unlikely that making this option default true will affect performance of the framework itself - the expensive reflection was already being performed. There's no new loops or branching. Impact on users' performance experience will be dependent on whether they have "other assembly" base tests that become newly valid. |
@ajryan : That's actually a yes and a no. We actually get all runtime methods for a type and then determine if its a test method here. After this change however, we would be performing this reflection check for APIs in System.Object (at the minimum) as well. We don't really know the numbers for |
@AbhitejJohn Ahh - I see your point. We would short-circuit any other assembly method and skip checking for attributes. Perhaps when the "other assembly" option is enabled it would be more efficient to perform the attribute check first. |
While trying to get the perf numbers out to make this the default, I hit into the following:
|
Here are the top level discovery numbers I got from VS for a solution with 9 projects each having 10 test classes each with 100 tests: With the config enabled: 4.67 s. (average of 5 runs) This is on a 16 Gb, 2 core machine. I'm wary of doing so however due to the 2nd issue above. @ajryan , do you want to give a shot at fixing that? It is to do with the assembly full path we pass in here. If we pass in the (base)assembly where the test method is actually defined this might be fixed. |
sure, I'll take a look and come back with questions
|
Sure. Here is little more context on how the 2nd part works: This code gets the line number and file name where a test is defined based on the source. We use DIA internally to populate this data(DIA in-turn works on pdb files). In our case, we need to be searching for the base test methods in the base assembly and not in the derived test assembly. After we make a change to set the appropriate source on a test element as I detailed above, we would also need to change this code to pass in the source from the test element so it gets the right data. Hope that makes sense. |
Thanks for the pointers, this makes sense. I think TestMethod should carry a DeclaringAssemblyName parallel to the way it exposes DeclaringClassFullName. I'll thread it through from the discovery end and then make sure the navigator can locate the source correctly. |
Just tried this today and it works great! Thanks for the hard work, guys! Few issues: When you upgrade the NuGet package to the .17 version, you have to restart Visual Studio before you can run tests again. Otherwise you get some error about a constructor not being found. No biggie. The inherited test methods in the base class must be public to get run; protected won't work. This probably isn't a huge deal, but perhaps should be noted somewhere. |
@ajryan: Yes, that should be great. There could be issues in figuring out the location of the assembly though, MSTest.CoreAdapter being a PCL. We might need to pass it through another API in IFileOperations.
This is an issue with a cache the Test Explorer maintains. It does not get refreshed - a bug we are tracking internally.
The test method signature does not like protected methods today. It looks like we need to reconsider that. |
This is now part of 1.1.17 on nuget.org. This isn't the default workflow in this release though (due to the issue being discussed above) and can be enabled using the following settings:
Happy unit-testing. :) |
About determining assembly location from TypeEnumerator in the PCL... I now understand your comment about about farming it out to PlatformServices/IFileOperations, I came to the same conclusion while I was working it through. For the concrete implementations:
I am doing exploratory testing locally in VS2017 with nuget packages built with my changes. I think things are looking good with the base methods appearing under the derived class assembly, meanwhile navigating to source of the base methods locates the correct file+line. One thing I noticed, somewhat related to the ClassInitialize discussion -- when executing an inherited remote assembly base method: ClassInitialize is called correctly on the base assembly class and a TestContext is passed, but WriteLine calls to that context are not captured as output of the base method. You can see my progress so far here: ajryan@5c0d2b6 |
That sounds great @ajryan . Looked through your changes as well - looks good.
We can surely move over to ns1.5. We just wanted to leave it at a minimal standard as a principle.
Oh I see, this would be coming out in one of the test methods of the derived class. Will file an issue for this one. I'm gonna re-open a new issue for this with all the data above - The issue in the title is actually already fixed.. |
Description
Steps to reproduce
Expected behavior
Actual behavior
Environment
The text was updated successfully, but these errors were encountered: