Third Party Test Runner Resiliency #302
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…to take part in the debugger-attached code path.
LaunchProcessWithDebuggerAttached would be silently skipped. Third-party test runners may or may not have the debugger attached to the currently running Fixie.TestAdapter process during the discovery phase. That is their choice and for some test frameworks that may even be meaningful. In Fixie's case, debugger-attached runs only make sense when the secondary test assembly process we want to launch is being launched by the IFrameworkHandle.LaunchProcessWithDebuggerAttached method. However, there is no IFrameworkHandle instance during any Discovery phase, so by here it is null. If we were to attempt to run the debugger-attachment code during discovery, where the IFrameworkHandle is null, the process launch would be silently skipped with the use of `?.`, and the user would experience errors while the interprocess communication between the currently running Fixie.TestAdapter process and the *nonexistent* secondary test assembly process inevitably failed.
…istinguishing when the IFrameworkHandle is null. This change will be improved upon in the next commit by renaming the overloads.
…t clear which code path is intended for each phase of the testing lifecycle.
…ges, to reveal dead code paths during the discovery phase.
…at the returned Process on this code path is never null.
…nitialization expression, enabling the Inline Parameter refactoring in the next commit.
…ion code paths by demonstrating that the test assembly's working directory can be inferred just prior to launching the test assembly process.
…ers with guidance to immediately continue debugging their test, as well as providing evidence to submit to the third party test runner's organization.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR ensures Test Adapter resiliency in the face of faulty third party
LaunchProcessWithDebuggerAttachedimplementations. It is a replacement for the earlier attempts at resiliency removed in #300 and #301.In the past, I made the mistake of attempting to work around such debugger-attachment issues to the detriment of the project. I added workarounds intended to smooth over third party gaps, in fact dropping end users into bizarre assembly loading issues instead of driving them directly to success. I added workarounds intended to allow expected behavior for as many test runners as possible, only to find later that the Test Adapter was throwing exceptions that left their UX stuck spinning or reporting "inconclusive" results. In each case, in addition to creating false solutions, the complexity in the implementation grew.
#300, #301, and this code change mark a change in the Fixie project's stance on test runner integrations. When the world insists on taking the "move fast and break things" stance, automated testing proponents must meet that world with the opposing stance: "move slow and heal things". That goes for our tests as well as our test frameworks.
To follow my own advice, this repo will no longer be interested in workarounds, when I can instead give the user immediate guidance that unblocks their ability to move slow and heal things, and when I can give the user sufficient evidence to report their (now non-blocking, low-stakes) issue to the correct party.
Background
Fixie provides a VSTest "Test Adapter" primarily in order to take part in the Visual Studio Test Explorer test runner. Any third party test runner can integrate by providing a "test host" process similar to the one provided by VSTest which calls into our Test Adapter to run tests and receive results.
For some test frameworks, that's enough to handle debugging. The test runner (such as an alternative IDE) launches their test host with the debugger attached to that process, loads the test framework's Test Adapter assembly, and calls into it with reflection (e.g. "Run All Tests Please"). If the test framework runs tests in-process, then test method breakpoints will be hit.
To ensure legitimate isolation, though, test frameworks can run the actual test assembly in a dedicated secondary process. For these cases, the Test Adapters need to call back to the test host (e.g. an IDE that just attached a debugger to the test host), asking them to start the process for them under the same debugging session. Doing so is not within the Test Adapter's control. (There are yet more reasons beyond environment and assembly loading isolation that a VSTest-integrated test framework might work with attaching debuggers to external processes, but they don't apply in this case.)
Specifically, any VSTest Test Adapter can make one pivotal request back to the test host: to launch a secondary process with the test host's debugger attached:
VSTest's own test host implements this method, and so debugging Fixie tests works in Test Explorer without user intervention. The secondary process is enlisted in the existing debugging session, and the user has no need to be aware of the process hops involved. 8000
JetBrains' VSTest runner for ReSharper and Rider, unfortunately, throws NotSupportedException from this method.
Prior to this PR, this meant that the Test Adapter would encounter a highly-unanticipated exception at a pivotal moment: while the Test Adapter is establishing a named pipe connection to the test assembly process. Interrupting that communication would then fail with exceptions of its own around the far side of the pipe never connecting, and the JetBrains UI would have little to no information about what went wrong.
Solution
This PR improves our error handling in these situations with the following goals: