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

Make PipTestEnvironment stderr checks more assertive #6871

Open
chrahunt opened this issue Aug 14, 2019 · 9 comments
Open

Make PipTestEnvironment stderr checks more assertive #6871

chrahunt opened this issue Aug 14, 2019 · 9 comments
Labels
C: tests Testing and related things state: awaiting PR Feature discussed, PR is needed type: maintenance Related to Development and Maintenance Processes

Comments

@chrahunt
Copy link
Member

We can tighten the tests in the following way. Rename the PipTestEnvironment.run() arguments of allow_stderr_warning and allow_stderr_error arguments to expect_stderr_warning and expect_stderr_error:

pip/tests/lib/__init__.py

Lines 501 to 507 in 2bfafc9

def run(self, *args, **kw):
"""
:param allow_stderr_warning: whether a logged warning (or
deprecation message) is allowed in stderr.
:param allow_stderr_error: whether a logged error is allowed in
stderr. Passing True for this argument implies
`allow_stderr_warning` since warnings are weaker than errors.

and make the test fail if the argument is passed and a warning or error isn't present, respectively. The idea is that the argument should only be passed if it's actually needed, unlike the current case where the test is passing if the argument is provided unnecessarily.

Also, to do this it might be necessary to do a check of some kind to know if the argument should be passed. For example, whether a deprecation warning is emitted can depend on what version is running.

Posted in its original form by @cjerdonek in #161 (comment)

@triage-new-issues triage-new-issues bot added the S: needs triage Issues/PRs that need to be triaged label Aug 14, 2019
@chrahunt chrahunt added C: tests Testing and related things state: awaiting PR Feature discussed, PR is needed type: maintenance Related to Development and Maintenance Processes and removed S: needs triage Issues/PRs that need to be triaged labels Aug 14, 2019
@cjerdonek
Copy link
Member

A related task is to remove the expect_stderr argument since it's redundant with allow_stderr_warning.

@atugushev
Copy link
Contributor

Hello @cjerdonek,

Could you help me to clarify a few cases?

Cases for expect_stderr_warning=True

run(command, expect_stderr_warning=True)
  1. if a command produces no stderr
    ... then raises a RuntimeError("expect a warning")
  2. if a command produces stderr: "WARNING: Hello, world!"
    ... then nothing raises
  3. if a command produces stderr: "ERROR: Hello, world!"
    ... then raises a RuntimeError("expect a warning")

Cases for expect_stderr_error=True

run(command, expect_stderr_error=True)
  1. if a command produces no stderr
    ... then raises a RuntimeError("expect an error")
  2. if a command produces stderr: "WARNING: Hello, world!"
    ... then raises a RuntimeError("expect an error")
  3. if a command produces stderr: "ERROR: Hello, world!"
    ... then nothing raises

Questions:

  • Are these statements true?
  • Should expect_stderr_error=True imply expect_stderr_warning=True?

@pradyunsg
Copy link
Member

pradyunsg commented Jan 10, 2020

My answers:

  • Yes.
  • No, it should require that.

@ssurbhi560
Copy link
Contributor

Hey @atugushev , can I please take your work further and work on this issue, if you are not onto this anymore? :-)

@atugushev
Copy link
Contributor

Hey @ssurbhi560. Thanks for pinging me! Can't really find a time to get this done. Would you like to pick it up? Please, feel free to 🙏

@ssurbhi560
Copy link
Contributor

@atugushev , I would be glad to pick it up and take this further.
Thank you ! :)

@gutsytechster
Copy link
Contributor

Hey @ssurbhi560 are you working over it? If not, then I'd like to take up this issue. :)

@ssurbhi560
Copy link
Contributor

Hey @gutsytechster! I won't be able to work on this right now, feel free to work on it. :)

@gutsytechster
Copy link
Contributor

Here, does it mean that for an error to be expected to raise, the variables expect_stderr_error and expect_stderr_warning would be both True? However, in case of an expected warning, only expect_stderr_warning would be True but not expect_stderr_error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: tests Testing and related things state: awaiting PR Feature discussed, PR is needed type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants