avocado icon indicating copy to clipboard operation
avocado copied to clipboard

Avoiding problems like "'StreamToQueue' object has no attribute 'isatty'" downstream

Open pevogam opened this issue 3 years ago • 3 comments

Discussed in https://github.com/avocado-framework/avocado/discussions/4953

Originally posted by pevogam September 17, 2021 As Avocado's runners are replacing the stdout and stderr of the tests with a custom StreamToQueue class, it might be a better idea if this StreamToQueue inherits from or has behavior that is more compatible with python's default stdout. A test script or test utility downstream might for instance do if stdout.isatty that will result in breakage because it might not necessarily assume Avocado has swapped the output channels (e.g. if the code is used in more general scopes beyond avocado). Do you think we should make the StreamToQueue objects more compatible with stdout objects or do you think there is a problem in the argument here?

pevogam avatar Jun 28 '22 10:06 pevogam

@clebergnu @beraldoleal @richtja I just had another trouble like this: an imported library that is used in the tests needed to use stderr in

[stdlog] 2022-06-28 11:08:07,666 avocado.test ERROR|   File "/usr/lib/python3.9/site-packages/graphviz/_compat.py", line 61, in stderr_write_bytes
[stdlog] 2022-06-28 11:08:07,666 avocado.test ERROR|     encoding = sys.stderr.encoding or sys.getdefaultencoding()
[stdlog] 2022-06-28 11:08:07,666 avocado.test ERROR| AttributeError: 'StreamToQueue' object has no attribute 'encoding'

where in a regular sys.stderr import we will have it:

[pevogam@drogon avocado-i2n]$ python3
Python 3.9.13 (main, May 18 2022, 00:00:00) 
[GCC 11.3.1 20220421 (Red Hat 11.3.1-2)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.stderr.encoding
'utf-8'

To keep it short, if we implicitly replace the standard streams we should make sure we provide maximum compatibility because literally any external library that is not aware it is run from an avocado test might break in such instances.

pevogam avatar Jun 28 '22 10:06 pevogam

@pevogam I think you are on the right track. This was an almost unsolvable problem with the legacy runner, because of its forking/execing model.

Now with nrunner, we are better positioned to remove the existence of StreamToQueue. The actual runners (say avocado-runner-exec-test) already capture the stderr/stdout of the executed process.

Let me know if this makes sense to you.

clebergnu avatar Jul 11 '22 18:07 clebergnu

@pevogam I think you are on the right track. This was an almost unsolvable problem with the legacy runner, because of its forking/execing model.

Now with nrunner, we are better positioned to remove the existence of StreamToQueue. The actual runners (say avocado-runner-exec-test) already capture the stderr/stdout of the executed process.

Let me know if this makes sense to you.

Removal also sounds good and adds the reduced maintenance costs on top so it makes complete sense. Should I rephrase this issue as "remove StreamToQueue" or would you like to create a separate issue about this concrete plan?

pevogam avatar Jul 12 '22 05:07 pevogam