Skip to content

Validate that stdout is not polluted and intercept shutdown requests from client in tests #1592

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

Closed
rjmholt opened this issue Oct 25, 2021 · 2 comments

Comments

@rjmholt
Copy link
Contributor

rjmholt commented Oct 25, 2021

After debugging integration test hangs for a few days for #1459, I discovered that test hangs were being caused by the following issue:

  • Some functions were still writing output to the PSHostUserInterface
  • We weren't changing the implementation of that to do nothing when ConsoleReplEnabled was false
  • That meant we'd write random strings to stdout, which confused the client
  • The client would interpret this as an error and send the server a shutdown request
  • The server would comply, but we wouldn't see anything in the test to corroborate this, so the test would just hang

I've since implemented a dummy host in the server to enable this scenario (but I don't love it). I've also added debug logging for the stdio streams so the stream content can be viewed when a debugger is attached.

To improve on this, we should:

  • Validate messages sent over the streams and throw if they are invalid
  • See if we can hook into the client sending a shutdown message so we can monitor that and again log or fail tests when it happens
  • Make the server exit the process when it's asked to shut down (not sure how to make this happen yet or I'd do it myself)
@ghost ghost added the Needs: Triage Maintainer attention needed! label Oct 25, 2021
@StevenBucher98 StevenBucher98 added Area-Test and removed Needs: Triage Maintainer attention needed! labels Oct 26, 2021
@andyleejordan andyleejordan moved this from Todo to P0 - Todo in American Pharoah May 3, 2022
@andyleejordan
Copy link
Member

I validated this! And fleshed out the null host.

Repository owner moved this from Todo to Done in American Pharoah May 19, 2022
@ghost
Copy link

ghost commented May 19, 2022

Thank you for your comment, but please note that this issue has been closed for over a week. For better visibility, consider opening a new issue with a link to this instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

No branches or pull requests

3 participants