Skip to content

Tighten up the test suite's stderr checks #6351

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

Merged
merged 4 commits into from
Mar 27, 2019

Conversation

cjerdonek
Copy link
Member

@cjerdonek cjerdonek commented Mar 20, 2019

This is a relatively simple PR that does the following four things (each in a separate commit):

  1. Makes deprecated_python pass allow_stderr_warning=True instead of expect_stderr=True. This narrows / tightens up the test expectation.
  2. Removes the logic for an old check for an SSLContext warning, since that is now subsumed by the deprecated Python check.
  3. Makes expect_stderr=True mean the weaker allow_stderr_warning=True. Similar to (1), this makes the test expectation more specific.
  4. Checks for a couple more incompatible argument combinations, which is similar to the suggestion @xavfernandez made to me on PR Allow finer-grained testing of subprocess stderr. #6342.

@cjerdonek cjerdonek added C: tests Testing and related things skip news Does not need a NEWS file entry (eg: trivial changes) labels Mar 20, 2019
@cjerdonek cjerdonek force-pushed the tighten-test-stderr-checks branch 3 times, most recently from 67fb4fe to a5c6662 Compare March 21, 2019 11:56
@cjerdonek cjerdonek requested a review from xavfernandez March 21, 2019 11:58
@cjerdonek
Copy link
Member Author

@xavfernandez This is now ready to be reviewed.

There are two things I think should be done in a follow-on PR:

  1. Change allow_stderr_error to have the meaning of expect_stderr_error (if possible, or if not, add expect_stderr_error as another option), and
  2. Change allow_stderr_warning to have the meaning of expect_stderr_warning (if possible, or if not, add expect_stderr_warning as another option).

Both of the above would make the tests more precise.

I tried doing (1) here, but I found that there are ~35-70 tests (depending on how much group 1 and 2 overlap) where expect_error is being passed seemingly unnecessarily (since no error actually occurs). Thus, it's a bigger change that I think should be done separately.

I don't know how many tests (2) would affect.

@cjerdonek cjerdonek changed the title [WIP] Tighten up the test suite's stderr checks Tighten up the test suite's stderr checks Mar 21, 2019
@cjerdonek cjerdonek force-pushed the tighten-test-stderr-checks branch from d8058a3 to e175817 Compare March 22, 2019 05:44
@@ -303,6 +303,9 @@ def script(tmpdir, virtualenv, deprecated_python):
assert_no_temp=True,

# Deprecated python versions produce an extra deprecation warning
# This also addresses a possible warning on old versions of Python
# (< 2.7.9) about the lack of an SSLContext in urllib3/requests,
Copy link
Member

Choose a reason for hiding this comment

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

This isn't clear: one could think that 2.7.9 is dealt with in deprecated_python but currently, only 2.7 and 3.4 are.
(and if the 2.7 warning is removed, it's unlikely someone will add a special case for 2.7.9...)
I think we can drop the comment about 2.7.9 altogether.

@cjerdonek cjerdonek force-pushed the tighten-test-stderr-checks branch from e175817 to 66ae681 Compare March 27, 2019 03:34
@cjerdonek cjerdonek merged commit de242d0 into pypa:master Mar 27, 2019
@cjerdonek cjerdonek deleted the tighten-test-stderr-checks branch March 27, 2019 05:19
@lock
Copy link

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.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation 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.
Labels
auto-locked Outdated issues that have been locked by automation C: tests Testing and related things skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants