refactor(server): make UpdateEncoder::update() an iterator

[elmarco]

A single display update can now result in multiple commands / update
code (FastPathUpdate).

The update dispatching and bitmap encoding is now done by the
UpdateEncoder itself.

(preliminary patches from #670)

[CBenoit]

issue: Let’s keep the tests in the testsuite crates.
https://github.com/Devolutions/IronRDP/blob/aeb1193674641846ae1873def8c84a62a59213d5/ARCHITECTURE.md#test-at-the-boundaries-test-features-not-code

[elmarco]

How can we test internal code ? I understand the will to focus on external boundaries, but why not allow internal stuff to be tested?

[CBenoit]

To enumerate a few reasons:

• Tests on internal API often ends up being "technical debt" slowing down future refactors.
  • Somewhat related good code is easy to delete, and such tests represent an investment on the existing code, making it harder to delete.
• Enabling Cargo "unit tests" slows down our CI. The explanation is written down in ironrdp-testsuite-core’s main.rs file IIRC.

The solution to all of this is to test at the boundaries instead.
My expectation is that it should be possible to test the same code by calling the public API of ironrdp-server.
Maybe the design of ironrdp-server is not making this straightforward though, since ironrdp-server is not following the sans-IO approach like the rest of the codebase.

I feel mixed because I don’t want the tests you wrote to just be deleted, but at the same time I really don’t want us to start writing this kind of tests everywhere, and moving away from this after the facts is difficult.

Maybe we can add a feature flag to expose a few high value internal APIs that we really want to test directly if it’s impractical to do it the "ideal way".

What is your opinion here?

To not further block you, I would be okay with merging now and addressing this point in a follow up.

[elmarco]

On one hand, I like the reasons you enumerate, but on the other hand it seems much harder to write tests at boundaries (and probably slower too). So I'd rather have internal tests, that we should feel free to ditch when doing refactoring than none or few. We could add an "extra-tests" cfg, but that won't help with CI time if we enable it.

What we can do in this particular "leaf" case is to link the server file in testsuite-core, see the last patch.

[CBenoit]

We could add an "extra-tests" cfg, but that won't help with CI time if we enable it.

What I’m suggesting is to expose the internal API for testing purposes from an external crate, not to enable tests conditionally.
This is helping with CI time, because we don’t have to build and link extra binaries, since all the tests are kept in the test suite.

The trick you are using in the last commit is also interesting though.
It feels hacky, but does not require an extra feature flag.
Instead of creating a symlink, maybe you can use the path attribute for the module?
I think this will work better on all platforms.

[elmarco]

"path" would not get the file packaged. I know we don't publish the testsuite, but this will probably have to change.

[CBenoit]

I don’t think we’ll ever package the test suite, unless you have a use case for that?

[elmarco]

I wouldn't call it a "use-case" :) but often distributions (for ex) help finding issues early on various environements, especially when package have tests. The more tests you can ship, the better. Kind of CI at a larger scale.

[CBenoit]

This sounds nice! But tests are not going to be part of individual packages for IronRDP. How would you go about this? I feel that it will be necessary to rely on something else than a "crate", and you would test the IronRDP "crate collection" as a whole. It’s not really clear to me what would be the most convenient for you, but I can tell that for now I’m a bit unwilling to publish all our internal packages to crates.io.

[CBenoit]

LGTM! Merging now, as there is no blocker.