Skip to content
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

TestSigProxyWithTTY: fix #2016

Merged
merged 1 commit into from
Jul 31, 2019
Merged

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Jul 24, 2019

exec.CombinedOutput() should not be used here because:

  • it redirects cmd Stdout and Stderr and we want it to be the tty
  • it calls cmd.Run() which we already did

While at it

  • use pty.Start() as it is cleaner
  • make sure we don't leave a zombie running, by calling Wait() in defer
  • use test.Name() for containerName

@codecov-io
Copy link

codecov-io commented Jul 24, 2019

Codecov Report

Merging #2016 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2016   +/-   ##
=======================================
  Coverage   56.79%   56.79%           
=======================================
  Files         311      311           
  Lines       21836    21836           
=======================================
  Hits        12402    12402           
  Misses       8519     8519           
  Partials      915      915

@kolyshkin kolyshkin changed the title TestSigProxyWithTTY: fix [WIP] TestSigProxyWithTTY: fix Jul 24, 2019
@kolyshkin kolyshkin force-pushed the TestSigProxyWithTTY branch 7 times, most recently from c3d7850 to ba3d3d8 Compare July 29, 2019 21:57
@kolyshkin kolyshkin changed the title [WIP] TestSigProxyWithTTY: fix TestSigProxyWithTTY: fix Jul 29, 2019
@kolyshkin kolyshkin requested a review from thaJeztah July 30, 2019 23:53
exec.CombinedOutput should not be used here because:
 - it redirects cmd Stdout and Stderr and we want it to be the tty
 - it calls cmd.Run which we already did

While at it
 - use pty.Start() as it is cleaner
 - make sure we don't leave a zombie running, by calling Wait() in defer
 - use test.Name() for containerName

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin kolyshkin force-pushed the TestSigProxyWithTTY branch from ba3d3d8 to e085cd3 Compare July 31, 2019 00:40
@kolyshkin kolyshkin changed the title TestSigProxyWithTTY: fix [WIP] TestSigProxyWithTTY: fix Jul 31, 2019
@kolyshkin kolyshkin force-pushed the TestSigProxyWithTTY branch from e085cd3 to bc4ed69 Compare July 31, 2019 00:56
@kolyshkin kolyshkin changed the title [WIP] TestSigProxyWithTTY: fix TestSigProxyWithTTY: fix Jul 31, 2019
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants