ATO-554 add BlockingIOError handler, tests and portalocker #12271
Conversation
changelog/12271.bugfix.md
Outdated
| @@ -0,0 +1 @@ | |||
| Fix `BlockingIOError` when running `rasa cli` on utterances with more than 5KB of text. | |||
There was a problem hiding this comment.
We should be more specific about which CLI command causes this issue (I think it's rasa shell, could also be rasa interactive), please double-check with @trammell.
tests/shared/utils/test_io.py
Outdated
|
|
||
| mock = Mock() | ||
| monkeypatch.setattr(builtins, "print", mock_print) | ||
| monkeypatch.setattr(rasa.shared.utils.io, "handle_print_blocking", mock) |
There was a problem hiding this comment.
If we're mocking the function we want to test, we're not actually testing it, but I'd suggest to at least assert that mock was called with the desired output (in addition to the call_count assertion).
Also pytest has a good fixture for capturing stdout: capsys (https://docs.pytest.org/en/7.1.x/how-to/capture-stdout-stderr.html), have you tried with that too?
There was a problem hiding this comment.
Tried with capsys but it captures only the text output to stdout/stderr and not the system calls to lock/unlock stdout. After trying with capsys and capfd I decided to mock it but there was no good way (that I know of) to assert that lock/unlock happened so I was left with this...
|
|
||
| # Can't manipulate the STDOUT file descriptor | ||
| # in pytest (it's a io.StringIO instance not a file) | ||
| def test_handle_print_blocking(monkeypatch: MonkeyPatch): |
There was a problem hiding this comment.
This is not a test of handle_print_blocking but rather of the function which is calling it. Please move this test to the proper module and rename it to match the test case of parent caller.
There was a problem hiding this comment.
Moved the tests to -> test_cli.py and renamed appropriately
| @@ -626,3 +625,21 @@ def is_subdirectory(path: Text, potential_parent_directory: Text) -> bool: | |||
| def random_string(length: int) -> Text: | |||
| """Returns a random string of given length.""" | |||
| return "".join(random.choices(string.ascii_uppercase + string.digits, k=length)) | |||
|
|
|||
|
|
|||
| def handle_print_blocking(output: Text) -> None: | |||
There was a problem hiding this comment.
We must add unit test for this function to make sure that:
- portalocker.Lock is called properly - mock
Lockfunction - AnsiToWin32 is called properly if
sys.platform == "win32" - print is called with proper
output,fileandflushparameters
|
One Note: |
|
@radovanZRasa I was not able to reproduce it 100% of the time It happens mostly on MacOS |
…large-1-k-utterance-length
…large-1-k-utterance-length
…large-1-k-utterance-length
|
🚀 A preview of the docs have been deployed at the following URL: https://12271--rasahq-docs-rasa-v2.netlify.app/docs/rasa |
Urkem
deleted the
ATO-554-blocking-io-error-in-rasa-cli-with-large-1-k-utterance-length
branch
Proposed changes:
Status (please check what you already did):
black(please check Readme for instructions)