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

Test assembly dependency resolution in .netcore #34

Closed
codito opened this issue Aug 31, 2016 · 11 comments
Closed

Test assembly dependency resolution in .netcore #34

codito opened this issue Aug 31, 2016 · 11 comments

Comments

@codito
Copy link
Contributor

codito commented Aug 31, 2016

Problem

  • Test platform expects all dependencies for a test project to be available in the output directory
  • Test platform always starts the testhost.exe with a specific testhost.deps.json. In this case, user's dependencies don't resolve appropriately

While this behavior is fine for desktop clr, for .netcore these premises may not work.

Impact of this bug

  • User will be able to run tests in testproject.dll only after a dotnet publish, so that all dependencies are copied to output directory. dotnet build doesn't copy all dependencies to output folder.
  • User will not be able to specify the specific dependencies that they want. E.g. because testhost.deps.json is used, System.Xml used by testhost will be loaded, although user wanted to use a later version of System.Xml. Multi-targeting will be hard
  • Adapter writers need to use custom lookup (Assembly.LoadFrom) for resolving test assembly dependencies (incl. the framework assemblies)

Proposal

(There is no change in the test platform behavior for test projects targeting net46 or desktop clr. We concentrate only on netcore scenarios below)

Execute testhost.dll with testproject.deps.json

In dotnet core, set of dependencies for a project testproject are resolved with testproject.deps.json file available in the output path of the project. Test platform should invoke testhost.dll with the deps file of testproject i.e. testproject.deps.json.

Since testhost.dll is always invoked with testproject.deps.json, it is not possible to reuse the testhost for running tests of all test projects. For each test project/container, a separate testhost process will be invoked. This will have a performance impact (compared to existing implementation where a single testhost process is used for running tests from all assemblies).

Adapter lookup

Adapters are referenced in the testproject as a exclude: compile dependency. Test platform will probe the adapter from paths in testproject.deps.json. The lookup convention remains unchanged (*.testadapter.dll are considered test adapters).

Note that user may still provide a logger (or data collector) extension with a path. This will require the logger to be published.

Every test project depends on TestHost package

Every test project will include a exclude: compile type dependency on Microsoft.TestPlatform.TestHost nuget package. During project build, nuget takes the responsibility to compute the dependencies required for both testproject and testhost and create the testproject.deps.json.

testhost.dll is always compiled with the lowest netcoreapp runtime. This allows it to be retargeted to any higher runtime as specified in testproject.deps.json.

CI/CDP scenario

If the tests are run on the same machine that has source enlisted and build artifacts, simple dotnet test should work.

For CDP, where the build artifacts are consumed, a dotnet publish is required while publishing the artifacts. This ensures all dependencies are available for dotnet vstest on the test machine. In this case, user needs to provide path to testproject.dll directly.

dotnet publish for a netcore target produces a netcoreapp1.0 directory with all dependencies. A runtime specific directory is created for net46 target.

Open questions

  • Does this meet requirements for asp.net test apps?

References

@codito
Copy link
Contributor Author

codito commented Aug 31, 2016

@AbhitejJohn @sbaid @pvlakshm @saikrishnav thoughts?

@harshjain2
Copy link
Contributor

@codito

User will be able to run tests in testproject.dll only after a dotnet publish, so that all dependencies are copied to output directory. dotnet build doesn't copy all dependencies to output folder.

CI build would require dotnet publish on test project so that all dependencies are bundled; since there isn't a nuget package directory

AFAIK, dotnet publish doesn't work until specific runtime is specified. Do we want to force the users to always specify runtime in their test projects?

@codito
Copy link
Contributor Author

codito commented Sep 1, 2016

@harshjain2 without a RID specified, dotnet publish publishes to netcoreapp1.0 directory for netcore target and net46\win7-x64 (or x86) directory for net46 target.

I've updated original comment. CI build (or strictly speaking CDP) requires publish only if the build step has already run on another machine, and test is running in a separate machine with the build artifacts.

@harshjain2
Copy link
Contributor

harshjain2 commented Sep 1, 2016

@codito
I'm a bit concerned about dependencies of Test Adapter.

Test Adapter can have dependencies on MSTest.ObjectModel and other nuget packages.

Those dependencies are taken care of if test project adds Test Adapter nuget as relevant dependencies will be added in the test dll's .deps.json file.

I am not sure what will happen if user forgets to add Test Adapter. It might work if, just by luck, test dll has all the dependencies required for test adapter. Otherwise it will bomb.

As Test Platform owner, we should have a strict guideline that Test Adapter nuget packages should always be added to the test project just to ensure the .deps.json of test dll has the required dependencies needed for Test Adapter as well.

@harshjain2
Copy link
Contributor

@codito

  1. Test host is a dependency of each test project
  2. Test host is not a dependency, bundled with vstest.console package

On this, in order to ensure the dependencies of testhost is taken care of, option 1 is recommended. That way, dependencies of testhost will be present in test dll's .deps.json which we are passing when starting testhost.

@harshjain2
Copy link
Contributor

@codito @AbhitejJohn Do we know the requirements for asp.net tests?

@AbhitejJohn
Copy link
Contributor

In Solution #2, packaging would bloat up the vstest.console bundle since it might end up having a testhost and its dependencies built for a lot of netstandard versions. #1 gives the ability to fragment this with the ability to plug in which ever version is appropriate as long as it understands the protocol. This can also be a nice host extensibility entry point where someone can specify their host via a nuget package and vstest.console can communicate with that host.

One thing thats biting me with solution #1 however is the pain of the user to add 3 nuget dependencies -> Framework, Adapter and the test host. Just a thought, can we somehow tie the adapter to have a dependency on test host? That could possibly work @codito .

@harshjain2 : What requirements are we talking about? Currently ASP.NET Core targets the same core CLR. I do not see any plan for post RTM here though: https://github.com/aspnet/Home/wiki/Roadmap .

@codito : Does the new csproj based project system allow users to add references directly to assemblies as well?

@codito
Copy link
Contributor Author

codito commented Sep 2, 2016

@harshjain2 re the dependencies of test adapter, they will cleanly resolve with testproject.deps.json. dotnet build will ensure this. Dependencies of testhost will always be in the current folder of testhost (a dotnet publish has already been run on testhost before packaging).

I think we should take a staged approach (since this is a architectural change): start with approach 2, then move to approach 1.

@AbhitejJohn @harshjain2 probably meant the assembly loading in asp.net. We will do a small spike for this.

Updated notes on adapter discovery based on deps.json above.

@pvlakshm
Copy link
Contributor

pvlakshm commented Sep 2, 2016

Will a runsettings ever be supported? I ask because that has an AssemblyResolution entry where you can specify locations where dependencies may be found.

@codito
Copy link
Contributor Author

codito commented Sep 2, 2016

@pvlakshm I think we need to support runsettings since it also has platform, test logger, data collector and adapter settings. AssemblyResolution entries may not be applicable to netcore framework (they are also not applicable to C++, Nodejs and other languages the runner supports).

Updated notes with two relevant specifications from dotnet-cli around runtime configuration and corehost.

@codito codito added this to the Preview 107.1 - 20160923 milestone Sep 22, 2016
@codito
Copy link
Contributor Author

codito commented Sep 23, 2016

Resolved with 4f34968.

@codito codito closed this as completed Sep 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants