psyinfra / onyo

text-based inventory system on top of git
ISC License
3 stars 5 forks source link

investigate "rich print" behavior and unify both print functions of UI class #542

Open TobiasKadelka opened 8 months ago

TobiasKadelka commented 8 months ago

@aqw found an error that appeared in one of the tests for onyo_get():

❱ pytest -vv
[...]
=================================================================================== FAILURES ===================================================================================
___________________________________________________________________________ test_onyo_get_no_matches ___________________________________________________________________________

inventory = <onyo.lib.inventory.Inventory object at 0x7fbf7aedb9d0>, capsys = <_pytest.capture.CaptureFixture object at 0x7fbf7ae094d0>

    @pytest.mark.repo_contents(
        ["one_that_exists.test", "type: one\nmake: that\nmodel: exists\nserial: test\nsome_key: value"])
    @pytest.mark.ui({'yes': True})
    def test_onyo_get_no_matches(inventory: Inventory,
                                 capsys) -> None:
        """`onyo_get()` behaves correctly when `match` matches no assets."""
        asset_path1 = inventory.root / "somewhere" / "nested" / "TYPE_MAKER_MODEL.SERIAL"
        asset_path2 = inventory.root / "one_that_exists.test"
        matches = [Filter("unfound=values").match]

        # `onyo_get()` is called, but no assets match
        onyo_get(inventory,
                 match=matches)  # pyre-ignore[6]

        # verify output contains no assets because nothing matched
        output = capsys.readouterr().out
>       assert "No assets matching the filter(s) were found" in output
E       AssertionError: assert 'No assets matching the filter(s) were found' in 'No assets matching the \x1b[1;35mfilter\x1b[0m\x1b[1m(\x1b[0ms\x1b[1m)\x1b[0m were found\n'

I can reproduce this error exclusively when I run all tests -- if I just run the one for the file with the error (REPO_ROOT=$PWD pytest onyo/lib/tests/test_commands_get.py -vv) the test is not raised.

In the test, we assert for a specific string, but ui.rich_print() adds some special characters to the output, hence the error.


541 changes the rich-print to a normal print, which removes the occurence of the error.

  1. However, since the error seems to depend on first running some CLI tests, there seems to be something bleeding over into the python-tests -- possibly something connected to the UI class/object. This should be investigated.
  2. We want to have just one ui.print() and remove ui.rich_print() on the long run, i.e. unify both functions into one that decides when to rich-print and when not.
  3. 55 wants to colorize certain output; this issue should be considered when unifying both functions.

bpoldrack commented 8 months ago

This issue is actually two:

  1. The word 'filter' in that message is colored, which we don't want. This is because apparently rich enables syntax highlighting by default. PR #543 fixes this.

  2. This failure doesn't happen to me and according to OP also not always, suggesting that something differs WRT whether capsys is written to by rich as if it was a terminal or not. This needs further investigation.

aqw commented 8 months ago

I'm going to share some notes here. I have made no progress on this, but have eliminated a few things:

I can confirm that running the test directly does not reproduce the problem:

pytest -vv onyo/lib/tests/test_commands_get.py 

And when I try to manually reproduce the exact order the tests ran in when they failed, it doesn't fail:

pytest -vv  -p no:randomly  onyo/commands/tests/test_init.py onyo/commands/tests/test_unset.py onyo/commands/tests/test_shell-completion.py onyo/commands/tests/test_mkdir.py onyo/commands/tests/test_cat.py onyo/lib/tests/test_commands_set.py onyo/lib/tests/test_Filter.py onyo/tests/test_main.py onyo/lib/tests/test_commands_mv.py onyo/lib/tests/test_commands_get.py 

I tried dozens of combinations when running the tests directly. I was never able to reproduce the error.

I am only able to reproduce it when I run all tests. I can confirm that it affects me even with randomly disabled:

pytest -vv -p no:randomly

Neither running reset on my terminal nor loading a new terminal helps.

I run foot, reporting as xterm-256color. I tried running under xterm (reporting as xterm) and it still failed there.

So... I don't think there's something particular to my terminal environment influencing this. Especially since Tobias sees this problem on macOS as well.

So I think it's a Python issue. But from where, is still unclear.

TobiasKadelka commented 6 days ago

Since there is currently a lot of discussion going on in regards to printing and colors (#706, #702), I read through this issue again. It is not fully related, but I am still wondering if the original issue still exists?

aqw commented 6 days ago

The issue still exists. We have both ui.print() and ui.rich_print().