Support an unchanged framebuffer in expectCompare

[johbo]

Did bump into an issue when calling captureScreen and expectScreen one after the other.

The observation was that expectScreen did hang.

Code snippet:

    client.captureScreen(os.path.join(base_path, 'screenshot.png'))
    client.expectScreen(os.path.join(base_path, 'screenshot.png'))

I think the issue is caused because the screen content did not change and is already present, so there is no need for the server to send any update.

If I understand the whole thing correctly, then this will lead to a situation where self.deferred is never triggered. As far as I am tell it would be triggered from _doConnection in rfb.py, which calls into commitUpdate.

My understanding of twisted's deferred mechanism and also of the vnc protocol are somewhat "rusty", so I would probably need a few hints to bring the attached idea for a fix into proper shape.

The basic idea to avoid the issue is this:

Since we have the screen already, we can call _expectCompare initially synchronously. It will do the check, and only if it does not match (or if we did not yet have screen in the first place) it would go into the loop based on deferreds and hope for getting an update from the server after requesting this via framebufferUpdateRequest.

[sibson]

Seems reasonable. Is there a case where we'd want to force an update before doing a compare? I can't think of one off the top of my head.

[sibson]

thanks, ✨ 🍰 ✨

[pmhahn]

The pipeline on the main branch is now failing: https://github.com/sibson/vncdotool/actions/runs/3669547124/jobs/6203455052#step:6:28

Interestingly enough I'm unable to run nose locally at all: all test just succeed unless I introduce a syntax error.

[sibson]

I'm trying to remove nose in favour of simply python -m unittests tests/unit/*.py. At this stage I don't think nosetests is offering too much benefit.

[pmhahn]

Despite the change from nose to plain unittest (or maybe pytest?) the test is still broken:

commit 7e0b49cdce6d65d4a30af1a4c330ca87cb9fcf47 (HEAD -> modernize)
Author: Philipp Hahn <hahn@univention.de>
Date:   Wed Dec 21 14:20:53 2022 +0100

    test: Fix regression #221

    `test_expectScreen()` calls `expectScreen()` calls
    `_expectFramebuffer()`, which changed behavior by #221:

    - previously it called `_expectCompare()` via a `Deferred`. As the
      unit-test runs without any Twisted reactor running, the real code for
      comparison was never executed.
    - now `_expectCompare()` is called synchronously (is a previous `screen`
      already exists) and fails, because the mocked Image cannot be used
      with `len()`:
    > if len(hist) == len(self.expected):
    > E TypeError: object of type 'Mock' has no len()

    For the unit test restore the old behavior and let `self.screen = None`
    remain to *not* trigger calling `_expectCompare()`.

diff --git tests/unit/test_client.py tests/unit/test_client.py
index f782da3..69e0145 100644
--- tests/unit/test_client.py
+++ tests/unit/test_client.py
@@ -90,8 +90,6 @@ class TestVNCDoToolClient(TestCase):
         cli._handleInitial()
         cli._handleServerInit(b" " * 24)
         cli.vncConnectionMade()
-        cli.screen = mock.Mock()
-        cli.screen.size = (1024, 768)
         fname = 'something.png'

         region = (0 ,0, 11, 22)