Partial and static classes as skipped

[nohwnd]

Describe the bug

Partial and static classes are skipped over silently when MSTest source generator is used (see #1837), we need to decide if:

1) errors should be reported from the generator or reported on runtime by engine 2) align analyzers with the behavior of the source gen, and old and new engine.

Steps To Reproduce

Create mstest test project, add a test class, make it partial. It won't generate tests.

Expected behavior

Unsupported classes are not skipped over silently.

Actual behavior

Additional context

AB#1992482

[nohwnd]

Originally reported by @jamescourtney

[Evangelink]

We put those constraints for TATF but there is no reason to have/keep them for MSTest. @nohwnd I expect the change to be relatively trivial if you want to try to fit that in!

[testplatform-bot]

✅ Successfully linked to Azure Boards work item(s):

• User Story 1992482

[nohwnd]

@Evangelink we cannot directly report errors to the compiler or can we? MSTest does not run static test classes, and so in that case what should I emit? Broken code that will end up with non-specific error?

If we cannot tell compiler directly "mstest001: static classes not supported", then I would rather code this into analyzer, and append to a list of errors in the source gen, and throw all those exceptions at runtime.

And if I do throw them on runtime should there also be switch for the user to unblock themselves and run with a risk?

[nohwnd]

I will take it, just advise me on the errors.

[Evangelink]

We can return analyzer diagnostics but this will be duplicating some work from the MSTest.Analyzers project. I would vote for simply skipping/ignoring things that are not correctly shaped.

Another alternative, to avoid duplication of work would be to return a single generic diagnostic that says "case unsupported" and suggest installing the analyzers but I don't think it's bringing much value.

For static classes, they are currently only supported (regular MSTest engine) for AssemblyInit/Cleanup so they should simply be ignored for the current source gen engine.

For partial classes, I haven't checked what current engine does but I would expect no problem as it's considered as merged for runtime. For the source gen, I also don't expect any issue as we are working on symbol level and partial defs are merged at this level.

[nohwnd]

Had a short call with Amaury, and we agreed that analyzers should be our source of truth here, and if you are able to build without warnings you should not be missing any tests. There won't be any additional runtime exception that blocks you from running and calls out the tests you are potentially missing.

Meaning in this case that putting static test method on the static test class will give you warning about the method not having the correct shape.

And there won't be any warning about partial classes. Tests from partial classes should be normally picked up.

[nohwnd]

Was solved in the linked PR, static classes are already covered by analyzers.