Validate state of the test node to help adapters to give always expected information to the extensions/data consumers

[MarcoRossignoli]

Ref #3478 (comment)

At the moment "the platform" is nothing more than a IPlatformOutputDevice that collects an print in console or in server mode sent states outside. We don't have validation on what adapters are sending. We could think to add a "strict" verification of the state of every test node at least from the perspective of the lifecycle expected by the platform "at minimum"(you can push what you want if other extensions understand your shared model, like trx), like "discovered(non mandatory) -> in-progress(not mandatory) -> executionstate(mandatory)" rejecting with an exception if you push out of this expected state. cc: @Evangelink

[bradwilson]

Just so that it's documented here, I raised the question of whether it's "illegal" (or, more specifically, troublesome) to the platform whether xUnit.net sent an "in-progress" message and then never got around to sending a result message, which in part inspired this issue.

Our generalized use case is: test framework (which is extensible) says "I'm starting to run this test", and then says "I've decided not to run this test" (aka, its result state is "NotRun"). This is contractually one of the result states that we support, in addition to the more traditional "Passed", "Failed", and "Skipped".

As the runner (which is separate from the test framework), we receive these messages at independent times: an ITestStarting, followed later by one of our result messages, and lastly by ITestFinished. We map ITestStarting to the in progress test node, and then we map the more traditional results (ITestPassed, ITestFailed, ITestSkipped) into the appropriate "execution state" nodes. There is nothing to map ITestNotRun onto in the Microsoft Testing Platform model. (The finished message is not used in this scenario.)

In practical terms, the "in-the-box test framework" (XunitTestFramework) designates when this situation will happen: if a test is marked as Explicit = true and we're running in ExplicitMode = off (the default), or if a test is marked as Explicit = false (the default) and we're running in ExplicitMode = only. But the runner cannot make these assumptions, because the test framework is a user extensibility point, and in fact it's only the test framework that knows anything about what defines a test at all. Our runner is, by design, as much in the dark about the inner workings of the test framework as Microsoft Testing Platform is in the dark about the inner workings of us.

So my original concern was: Is it an error condition to send Microsoft Testing Platform an "in progress" node for a test, and then never send the "execution state" node for that test?

The additional data point that made me ask that is this: Microsoft Testing Platform does not have a "not run" execution state, but the Test Explorer UI most definitely has a "not run" execution state (it's the state all tests are in before they've been run once). Using TPv2 today, we preserve this "not run" execution state successfully; using Microsoft Testing Platform, the only way for us to preserve the "not run" execution state is to simply not send Microsoft Testing Platform an "execution state" node. Sending any of the three legal "execution state" nodes would mark the test visually as either passed, failed, or skipped; none of those is technically correct. So we just don't send anything, in an attempt to preserve the proper Test Explorer UX.

Anyway, that's the background from the xUnit.net perspective.

---

Here is our command line experience vs. the MTP command line experience w/rt not run tests:

$ .\src\xunit.v3.assert.tests\bin\Debug\net472\xunit.v3.assert.tests.exe -explicit only
xUnit.net v3 In-Process Runner v0.3.0-pre.25-dev+759dee5cf8 (64-bit .NET Framework 4.8.9261.0)
  Discovering: xunit.v3.assert.tests (method display = ClassAndMethod, method display options = None)
  Discovered:  xunit.v3.assert.tests (912 test cases to be run)
  Starting:    xunit.v3.assert.tests (parallel test collections = on [24 threads], stop on fail = off, explicit = only, seed = 1091710143, culture = invariant)
  Finished:    xunit.v3.assert.tests
=== TEST EXECUTION SUMMARY ===
   xunit.v3.assert.tests  Total: 1056, Errors: 0, Failed: 0, Skipped: 0, Not Run: 1056, Time: 0.135s

$ .\src\xunit.v3.assert.tests\bin\Debug\net472\xunit.v3.assert.tests.exe --explicit only
xUnit.net v3 Microsoft.Testing.Platform Runner v0.3.0-pre.25-dev+759dee5cf8 (64-bit .NET Framework 4.8.9261.0)


Test run summary: Zero tests ran - src\xunit.v3.assert.tests\bin\Debug\net472\xunit.v3.assert.tests.exe (net472|x64)
  total: 0
  failed: 0
  succeeded: 0
  skipped: 0
  duration: 371ms

[MarcoRossignoli]

Thanks @bradwilson for the sample

[nohwnd]

I can come up with many arguments for and against this approach. So I am undecided. :)

Having a "strict" mode where we can account for the amount of tests is nice, but as you describe we don't know about how tests are being executed. We can only assume the most basic things like: a failed test means a failed run, but even that falls apart when we introduce test re-runs.

Similarly for tests bound to dynamic data (like query from a db), there can be different count per run. So this all only works if we would send list of tests to run, and expected getting all of them back, but that still has downsides (as we can see in VS Test Explorer that adopted this approach some time ago), you have to serialize and send all the test cases, account for the dynamic data tests that might not be the same as before, and account for tests that were added in the meantime.

[bradwilson]

Similarly for tests bound to dynamic data (like query from a db), there can be different count per run.

Unstable data sets are especially problematic for pre-enumerated data, so frequently commented on that I wrote a FAQ page about it. 😂 https://xunit.net/faq/theory-data-stability-in-vs

[bradwilson]

(as we can see in VS Test Explorer that adopted this approach some time ago)

Does this mean my page is no longer necessary? I haven't had anybody complain about unstable data sets in quite a while, so maybe the problem disappeared and I never noticed. 😂 Until TestDriven.NET decided to stop shipping versions, that was my preferred test runner, so my personal use of Test Explorer is fairly recent.

[nohwnd]

We believe there are two potential fixes for this. In our mind, the better choice would be for Visual Studio to use the "run just these tests" API when the user asks to "Run All", passing the list of previously discovered tests back to the test framework. This API already exists, and frameworks already have to implement it, so there should be no realistic downside to make this fix.

Yes that is how it currently works in VS with vstest, you send a full list of previously discovered tests to the runner to run them. In your case you serialize the parameters and recieve them back so tests run "as before", and re-running the tests will be deterministic, even though it will be outdated if the underlying source of data is dynamic.

Testing platform imho does not do this, and will re-run all with a filter. Preferring the data being up to date, rather than tests that TE already knows about being re-runnable.

[bradwilson]

The new filter will end up skipping new data as well as data that no longer exists, because having only the test case ID and not the serialized test case means we must perform a rediscovery, so unfortunately we will end up in the same scenario as “run all”: new data ignored (though at least not run), and old data that no longer exists will not be run despite ostensibly being in the requested filter.