Skip to content

Raise an exception on end of input in CliRunner #2934

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.

Sign up for GitHub

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

Jump to bottom
Merged
davidism merged 1 commit into from
May 20, 2025
Merged

Raise an exception on end of input in CliRunner #2934

davidism merged 1 commit into from
May 20, 2025
+27 −6

Conversation

@kieranyyu
Copy link
Contributor

@kieranyyu kieranyyu commented May 20, 2025

When end of input is reached in CliRunner, _NamedTextIOWrapper.readline continues to return an empty string which does not match the behavior when running the program at the command line (in this case an EOFError is raised which causes an Abort to be raised).

Using next() instead of readline will raise StopIteration from TextIOWrapper. If we want to perfectly match the behavior of regular input, we could override __next__ to raise EOFError (this will require a type: ignore due to mypy comparing types with IOBase instead of TextIOWrapper).

fixes #2787

kieranyyu
kieranyyu commented May 20, 2025
View reviewed changes
tests/test_termui.py
click.echo(f.name)

result = runner.invoke(cli)
result = runner.invoke(cli, input="\n")
Copy link
Contributor Author

@kieranyyu kieranyyu May 20, 2025

Choose a reason for hiding this comment

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

Newline needs to be inputted to simulate user input

@davidism davidism added this to the 8.2.1 milestone May 20, 2025
@davidism
Copy link
Member

davidism commented May 20, 2025

I think we do want to raise EOFError, since the issue says that this is what prompts are handling. Could do it with:

try:
    next()
except StopIteration:
    raise EOFError

kieranyyu
kieranyyu commented May 20, 2025
View reviewed changes
src/click/testing.py
def mode(self) -> str:
return self._mode

def __next__(self) -> str: # type: ignore
Copy link
Contributor Author

@kieranyyu kieranyyu May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

mypy claims the return type should be bytes because of IOBase but it should be matching with TextIOWrapper
Same issue as python/mypy#9643

@kieranyyu kieranyyu force-pushed the handle-extra-prompts branch from e8fd432 to e4f590e Compare May 20, 2025 20:26
@davidism davidism force-pushed the handle-extra-prompts branch from 26ce633 to 80efdf6 Compare May 20, 2025 23:04
@davidism davidism merged commit 262bdf0 into pallets:stable May 20, 2025
10 checks passed
asottile-sentry added a commit to getsentry/sentry that referenced this pull request May 21, 2025
@asottile-sentry
ref: fix test with not enough input
35e7ddf 
pallets/click#2934
@olasd olasd mentioned this pull request May 22, 2025
Closed
@jacobromero jacobromero mentioned this pull request May 22, 2025
Merged
1 task
timoffex pushed a commit to wandb/wandb that referenced this pull request May 22, 2025
@jacobromero
chore: fix failing unit tests due to click dependency update (#9891)
caeb77e 
Description
-----------
<!--
Include reference to internal ticket "Fixes WB-NNNNN" and/or GitHub issue "Fixes #NNNN" (if applicable)
-->

What does the PR do? Include a concise description of the PR contents.

This PR fixes consistent test failures related to `wand docker` cli tests. This was caused by an update with our dependency `click` in version `v8.2.1`, which changed the behavior with how the `CliRunner` is handling input when there is a prompt but no more input provided. (see click [issue here](pallets/click#2934), and the [fix here](pallets/click#2934))

<!--
NEW: We're using a new changelog format that's more useful for users. Please
see CHANGELOG.unreleased.md for details and update on relevant changes such as feature
additions, bug fixes, or removals/deprecations.
-->
- [ ] I updated CHANGELOG.unreleased.md, or it's not applicable


Testing
-------
How was this PR tested?
- running unit tests for `tests/unit_tests/test_cli.py`


<!--
Ensure PR title compliance with the [conventional commits standards](https://github.com/wandb/wandb/blob/main/CONTRIBUTING.md#conventional-commits)
-->
dmitryduev pushed a commit to wandb/wandb that referenced this pull request May 22, 2025
@jacobromero @dmitryduev
chore: fix failing unit tests due to click dependency update (#9891)
35d1699 
Description
-----------
<!--
Include reference to internal ticket "Fixes WB-NNNNN" and/or GitHub issue "Fixes #NNNN" (if applicable)
-->

What does the PR do? Include a concise description of the PR contents.

This PR fixes consistent test failures related to `wand docker` cli tests. This was caused by an update with our dependency `click` in version `v8.2.1`, which changed the behavior with how the `CliRunner` is handling input when there is a prompt but no more input provided. (see click [issue here](pallets/click#2934), and the [fix here](pallets/click#2934))

<!--
NEW: We're using a new changelog format that's more useful for users. Please
see CHANGELOG.unreleased.md for details and update on relevant changes such as feature
additions, bug fixes, or removals/deprecations.
-->
- [ ] I updated CHANGELOG.unreleased.md, or it's not applicable


Testing
-------
How was this PR tested?
- running unit tests for `tests/unit_tests/test_cli.py`


<!--
Ensure PR title compliance with the [conventional commits standards](https://github.com/wandb/wandb/blob/main/CONTRIBUTING.md#conventional-commits)
-->
the-13th-letter added a commit to the-13th-letter/derivepassphrase that referenced this pull request May 29, 2025
@the-13th-letter
Mitigate testing discrepancies from click 8.2.0 and below
62c0d29 
A discrepancy exists between the documentation of `click.prompt` and the
actual behavior of `click.prompt` when mocked with
`click.testing.CliRunner` on click 8.2.0 and below
([`pallets/click#2934`][BUG2934]): when prompting for input and if at
end-of-file, the `CliRunner` may return an empty string instead of
raising `click.Abort`. This usually translates to extra line breaks in
the "mixed" and "echoed" runner output at the last prompt(s).

We mitigate this discrepancy from both sides. On the code side, we wrap
each call to `click.prompt` to treat aborts and empty responses the
same, which is appropriate behavior for the types of prompts we issue.
On the test side, we amend our existing tests to use empty-line input
instead of no input, and explicitly test the "no input" scenario
separately, accepting both the 8.2.0-or-lower or the 8.2.1-or-higher
output. Because the behavior depends on the `click` version, which is
beyond our control, we also adjust coverage measurement.

[BUG2934]: pallets/click#2934
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

8.2.1

Development

Successfully merging this pull request may close these issues.

2 participants

@kieranyyu @davidism