search

nunit / nunit3-vs-adapter

NUnit 3.0 Visual Studio test adapter for use under VS 2012 or later
https://nunit.org
MIT License
203 stars 105 forks source link

Initial support of Microsoft.Testing.Platform #1153

Closed Evangelink closed 5 months ago

Evangelink commented 7 months ago

Provide the base changes to support the new platform while keeping backward compatibility.

There are multiple points to discuss for the design, naming and update of your testing infra.

Fixes #1152

MarcoRossignoli commented 7 months ago

:shipit:

SeanKilleen commented 7 months ago

I'd personally like to see https://github.com/microsoft/testfx/issues/2298 resolved before we move forward with this, to ensure that Microsoft is actually embracing a Framework agnostic runner vs trying to extinguish other frameworks (the comment referenced in that issue is...not encouraging.)

It may have already been resolved in a satisfactory way (Microsoft.Testing.Platform seems to be in the naming conventions now and the repo is testfx), but I'd like to confirm it.

Evangelink commented 7 months ago

@SeanKilleen I can already close the issue you are referencing. As you can see in the nuget packages added everything is already MSTest agnostic. The only thing that remains MSTest oriented is the main doc.

On the contrary to VSTest which is the main tool people use, the new platform is hidden and not something people are much aware of.

SeanKilleen commented 7 months ago

@Evangelink great, thanks for confirming!

Evangelink commented 7 months ago

@OsirisTerje @SeanKilleen How do you guys want me to move on this PR? Not sure if you prefer to discuss on the PR or ticket.

  1. Are you happy with having the new platform has optional disabled by default on the same nuget package or would you like to have a different package with maybe the runner enabled by default?

  2. If we move on with a flag to enable, how do you want to name it (current draft uses EnableNUnitRunner)?

Not for now but for some future, I'd be happy to start seeing the "VS" part going away (maybe if/when we are able to drop support from VSTest and focus only on the new platform). We are really trying to have the new platform felt more as a dotnet tooling/sdk concept rather than a VS centric feature.

BTW if that's any easier, I'd be happy to setup some call with you (hopefully we have some common timezone time :)) so we can discuss how we can help you.

OsirisTerje commented 7 months ago

So I'll simply copy your questions here over to the ticket and start answering there.

PS: Are the relevant packages now published so we could drop the local feed and get the CI build green?

Evangelink commented 6 months ago

Here is my build and sample testing workflow:

  1. build.cmd
  2. build.cmd -t package
  3. cd samples\1-nunit-runner
  4. dotnet build
  5. .\bin\Debug\net6.0\nunit-runner.exe
  6. cd ..\2-nunit-runner-dotnet-test\
  7. dotnet test

Could you confirm this is working for you?

OsirisTerje commented 6 months ago

I see the builds are still failing, due to the local nuget path.

D:\a\1\s\src\NUnit3AdapterExternalTests\NUnit3AdapterExternalTests.csproj : error NU1301: The local source 'C:\src\nunit3-vs-adapter\package' doesn't exist. [D:\a\1\s\NUnit3TestAdapter.sln]
OsirisTerje commented 6 months ago

I changed the nuget.config to point to a relative path image

But as the packages are not there, the build fails. image

I believe the best is to upload the packages to e.g. MyGet or as beta packages to nuget.

If I remove the Local feed, the solution builds, so they don't seem to be needed for the adapter ? You mention "for the samples". The nuget.config needs to go there, see below.

Also, the samples folder should be moved into either the NUnit-*-Samples repo or the docs repo. @SeanKilleen - what do you think?

Your pt.1 and 2 works Running tests give 1 failed, image

Meaning backwards compatibility is gone.

The acceptance tests, we have 72 failed out of 98, but this is our fault, as it is caused by mismatch of our tests with version 4 of NUnit. Need to add an issue to get that fixed :-) We need these tests for this PR.

I added new nuget.config files in the samples pointing up to the package directory.

Pt 3 -7 then works.

The output from 4: image

The output from 7:

image

No information here about tests however, except that "they" succeeded.

Evangelink commented 6 months ago

I changed the nuget.config to point to a relative path

Thanks, I called out this was not workin and was only a temp solution to demo you something so we can start some discussion.

Also, the samples folder should be moved into either the NUnit-*-Samples repo or the docs repo

Same as above, this is just a temp "solution" to demo. I expect quite some cleaning to do here.

Output from 4 is using vstest output (old dotnet test) while output from 7 uses the new default.

No information here about tests however, except that "they" succeeded.

This is to reduce output contention by default. We know this is not the most loved solution and we have this issue https://github.com/microsoft/testfx/issues/2162 to track improving this output for "interactive" scenarios.

For the tests, I expect you may want to "improve" them to run maybe in the 2 modes (runner or vstest) for better safety.

OsirisTerje commented 6 months ago
  1. I have copied the Samples folder into the adapter issues repo, at https://github.com/nunit/nunit3-vs-adapter.issues/tree/master/Issue1152.

[ ] It should be removed from this PR. [ ] Referring to the package can be done from a local location, e.g. C:\nuget, or pretty soon now, from Myget.

  1. We depend on the objectmodel version 11. This is for backward compatibility down to VS 2012. (We actually don't know anymore which VS version the adapter works for currently.) And we don't have any metrics either, nor any issues raised wrt this. I assume you want 4.6.2 to be able to run the new Testing.Platform, and that is fine. That should work enabling the later objectmodel, but when it runs in the "old" mode, it should still run version 11. I believe it will make for some interesting configurations.

  2. Please update this PR with the current master branch. That should make the PR pass the Cake CI build.

  3. Then please redirect this PR to the branch named vnext. It is currently pointing to the same commit in master. We can then use this branch as an integration branch, and also deploy an alpha-version from that branch. It will also make it easier for us to work on this.

Evangelink commented 5 months ago

@OsirisTerje I have commented ObjectModel v11 in the test because that's not clear how you want to handle that case.

I have been discussing with @nohwnd and having dependency on newer version of ObjectModel should not break support for older VS.

OsirisTerje commented 5 months ago

@Evangelink If it doesn't crash the older ones, we can remove it. We just have to trust you on that one. :-)

I no longer have any of those installed, and not sure if it will break on other things, like what framework we're actually now running under, so, anyway... Then we let that go.

OsirisTerje commented 5 months ago

The CI breaks due to the nuget local src, used for the samples. That should be removed. I have already moved the samples themselves, so when this is merged (and pushed to myget using the Publish build), the samples should grab the new version from myget.org

Evangelink commented 5 months ago

@Evangelink If it doesn't crash the older ones, we can remove it. We just have to trust you on that one. :-)

It shouldn't said our VSTest expert :)

The CI breaks due to the nuget local src, used for the samples. That should be removed. I have already moved the samples themselves, so when this is merged (and pushed to myget using the Publish build), the samples should grab the new version from myget.org

Yep sorry, will fix that now.

Evangelink commented 5 months ago

Working on a fix for the tests.