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

Test call_subprocess more thoroughly; fix spinner edge cases #6312

Merged
merged 2 commits into from Mar 12, 2019

Conversation

@cjerdonek
Copy link
Member

@cjerdonek cjerdonek commented Mar 3, 2019

The main purpose of this PR is to test call_subprocess() more thoroughly for a subsequent PR.

However, in the process I noticed a couple edge cases with the spinner that this PR fixes. They are--

  1. The spinner says error if the return code was non-zero but was one of the passed extra_ok_returncodes (and so should be acceptable). The spinner should instead say done.

  2. The spinner reports a final done / error finishing message even if the spinner wasn't actually used (e.g. in the case that show_stdout was true).

Both of these fixes can be seen in the changes in the test expectation in the second commit of this PR.

@cjerdonek cjerdonek force-pushed the subprocess-tests-and-spinner-fixes branch from f5b9256 to cd5984d Mar 3, 2019
@cjerdonek cjerdonek force-pushed the subprocess-tests-and-spinner-fixes branch 3 times, most recently from 6edb20b to c7f8a2b Mar 3, 2019
@cjerdonek cjerdonek force-pushed the subprocess-tests-and-spinner-fixes branch from c7f8a2b to 9a4967b Mar 3, 2019
@cjerdonek cjerdonek force-pushed the subprocess-tests-and-spinner-fixes branch from 9a4967b to fe79372 Mar 3, 2019
@cjerdonek
Copy link
Member Author

@cjerdonek cjerdonek commented Mar 10, 2019

@pradyunsg Would you be able to take a look at this PR? It mostly consists of adding tests (the first commit), but it also fixes a couple spinner edge cases (the second commit).

Loading

@pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Mar 12, 2019

Yep.

Loading

Copy link
Member

@pradyunsg pradyunsg left a comment

I'm okay with these changes.

Loading

@cjerdonek
Copy link
Member Author

@cjerdonek cjerdonek commented Mar 12, 2019

Thanks, @pradyunsg.

Loading

@cjerdonek cjerdonek merged commit 1599d65 into pypa:master Mar 12, 2019
29 checks passed
Loading
@cjerdonek cjerdonek deleted the subprocess-tests-and-spinner-fixes branch Mar 12, 2019
@lock
Copy link

@lock lock bot commented May 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Loading

@lock lock bot added the S: auto-locked label May 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants