Add E2E tests for the msentraid broker

[denisonbarbosa]

We've been needing those for a while now. Although they're not fully stable and automated yet, we should add the tests here to follow up with the other tasks and to discuss implementation details.

More details are explained in the individual commits.

UDENG-7424

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.33%. Comparing base (868dd8a) to head (1e6b111).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #659   +/-   ##
=======================================
  Coverage   78.33%   78.33%           
=======================================
  Files          22       22           
  Lines        1274     1274           
=======================================
  Hits          998      998           
  Misses        276      276           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[3v1n0]

Should we wait here or in the caller?

[denisonbarbosa]

I think we are sleeping at both... The sleep here is to make sure we wait for the request to fully finish before exiting it.

[3v1n0]

Ok, that's acceptable... But... We should never sleep this way in UI applications, but rely instead on the main loop, so can do instead something like:

def wait_for_timeout(timeout):
    loop = GLib.MainLoop()

    def on_timeout():
        loop.quit()
        return False

    GLib.timeout_add(timeout, on_timeout)
    loop.run()

[denisonbarbosa]

I feel like that is what the wait_for_stable_page function was supposed to be doing. Maybe we have a logic flaw somewhere?

[3v1n0]

wait_for_stable_page is only waiting if there are no page updates, we can maybe tune the timeout though.

[3v1n0]

Do we want to run it with RUNNING_OFFSCREEN or we prefer to see the UI to record it, just in case?

[denisonbarbosa]

Sometimes it bugs out and types the password in the incorrect field (I couldn't understand why exactly), but it's something that we definitely do not want to record

[3v1n0]

Mh, I see... One thing I wanted to add is the ability to save the page content on failure and we can do quite easily, but it was just a quick hack at the time :)

[denisonbarbosa]

That would definitely be nice to have. I used the script pretty much as you wrote it, so we can definitely improve some stuff there (although I don't really have experience with the GTK flow)

[3v1n0]

Let's keep this open, and see if I can handle it :)

[3v1n0]

Great stuff, I hope it's quite stable but we have a great base now

[3v1n0]

Not needed

[3v1n0]

Do we need to keep this?

[denisonbarbosa]

oops, forgot to remove it

[3v1n0]

Isn't sleep already part of matching (600)? Why sleeping more?

[denisonbarbosa]

Not really... We need the sleep to make sure we don't match the "cmd-finished" of the command itself

[3v1n0]

But didn't we clear it? I'd say we should do something command & clear && wait && echo cmd-finished, so extra sleep should not be needed

[denisonbarbosa]

We do clear it, but there's a sync issue between the commands. If the VM command does not run fast enough and YARF progresses to the next instruction, it can match the text before the screen gets updated.

[3v1n0]

I see, BTW please check with the new syntax and let's see if we can get such race, in case we can add the sleep back (even less than a a second though)

[denisonbarbosa]

AFAIK, there isn't any other way to avoid this issue. It's the same thing that's done in the installer tests, if I'm not mistaken.

[3v1n0]

Is this used anywhere?

[3v1n0]

Can this be moved to an utility function too?

[denisonbarbosa]

I think it could, but the code spawns a subprocess on itself, so I think it's good to have a "in your face" approach with it, wdyt?

[3v1n0]

It was mostly for avoiding to copy this for each test case we'll have (vanilla broker)

[3v1n0]

Not a big deal, but I feel you can have this with 4 lines of argparse

[denisonbarbosa]

I thought about it, but it's not worth adding the dependency for something as simple as this, especially for a script that is not intended to be run by people.

[3v1n0]

FYI argparse is part of the standard library

[denisonbarbosa]

Ok, it definitely shows that it's been a while since I've written Python 😂

[3v1n0]

I feel you can save even more by dropping the whole gnome-* runtimes

[denisonbarbosa]

Not necessary, IMO. The cached image is already small enough. If we need more space in the future, we could that.

[3v1n0]

Check it out, since these are quite big eh:

-rw------- 2 root root 517M set 27 02:27 /var/lib/snapd/snaps/gnome-42-2204_226.snap
-rw------- 2 root root 619M set  9 10:23 /var/lib/snapd/snaps/gnome-46-2404_125.snap

[3v1n0]

Maybe can use the exact code that is in https://github.com/ubuntu/authd/blob/main/docs/reference/cloud-init-deploy.md ?

[denisonbarbosa]

Sure. I'll update it (didn't even know we had a guide for cloud-init deployment)

[3v1n0]

Can drop /usr/bin and maybe call xdg-terminal, since we are not tergetting noble anyways here

[denisonbarbosa]

Even though we are not targeting Noble (yet, at least), I think it's better to keep the tests as release-independent as possible. If we change that, we would need to have multiple versions of the resource files (and although this might eventually become the case in the future, we should avoid it for now)

[3v1n0]

As you prefer, the /us/bin/ drop still stands :)

[denisonbarbosa]

Done!

[adombeck]

Is chrony even installed? It's not installed by default on Ubuntu AFAICT, and I don't see it being installed explicitly, so if it installed I wonder which package pulls it in as a dependency.

Can't we just use systemd-timesyncd?

Suggested change

	Run Command In Terminal sudo service chronyd restart && sudo chronyc waitsync
	Run Command In Terminal sudo systemctl restart systemd-timesyncd.service

[denisonbarbosa]

It's an Ubuntu server image, so it comes with chrony rather than systemd-timesyncd and they are mutually exclusive

[adombeck]

Ah ok, I didn't know that. That seems like the right approach then. Let's just use the systemd executable instead of the sys V init one to restart the service:

Suggested change

	Run Command In Terminal sudo service chronyd restart && sudo chronyc waitsync
	Run Command In Terminal sudo systemctl restart chronyd && sudo chronyc waitsync

[3v1n0]

I guess we can avoid these repetitions

[3v1n0]

Comment here is unneeded, it's more than clear.

However, maybe we can assert that window.is_drawable() and window.get_mapped()

[3v1n0]

I'd call it: save_window_snapshot

[3v1n0]

Suggested change

	if len(sys.argv) < 4:
	print(f"Usage: {sys.argv[0]} <username> <password> <code> [<output-dir>]")
	sys.exit(1)
	username = sys.argv[1]
	password = sys.argv[2]
	device_code = sys.argv[3]
	output_dir = sys.argv[4] if len(sys.argv) > 4 else "."
	screenshot_dir = os.path.join(output_dir, "webview")
	os.makedirs(screenshot_dir, exist_ok=True)
	try:
	login(username, password, device_code, screenshot_dir)
	finally:
	write_video(screenshot_dir, os.path.join(output_dir, "webview_recording.webm"))
	parser = argparse.ArgumentParser()
	parser.add_argument("username")
	parser.add_argument("password")
	parser.add_argument("device_code")
	parser.add_argument("--output-dir", required=False, default=os.path.realpath(os.curdir))
	args = parser.parse_args()
	screenshot_dir = os.path.join(args.output_dir, "webview")
	os.makedirs(screenshot_dir, exist_ok=True)
	try:
	login(args.username, args.password, args.device_code, screenshot_dir)
	finally:
	write_video(screenshot_dir, os.path.join(args.output_dir, "webview_recording.webm"))

[3v1n0]

While this is taking the snapshots and could be fine for some basic debugging, it doesn't catch potential problems IMHO as it relies on us taking snapshots.

So, you can instead just call save_window_snapshot on window draw signal, so that you can write all the frames (or a subset of them, maybe one once second is fine), and you'll get a proper video

[3v1n0]

Suggested change

	GLib.source_remove(poll_id)
	GLib.source_remove(poll_id)
	poll_id = 0

[3v1n0]

They're int values

Suggested change

	poll_id = None
	timeout_id = None
	poll_id = 0
	timeout_id = 0

[3v1n0]

Instead of doing this here, I'd return False here and instead restart the polling (calling poll_id = GLib.timeout_add(poll_interval_ms, poll_fn) after the finish function has returned, so that there's no possibility that we're polling twice while the same async function has not returned yet.

[3v1n0]

Suggested change

	loop.quit()
	def on_js_finished(web_view, result, user_data):
	nonlocal poll_id, timeout_id, found
	try:
	res = web_view.run_javascript_finish(result)
	js_value = res.get_js_value()
	found = bool(js_value.to_boolean())
	except Exception as e:
	print(f"Failed checking page JS: {e}", file=sys.stderr)
	pass
	if found:
	if timeout_id:
	GLib.source_remove(timeout_id)
	timeout_id = 0
	GLib.source_remove(poll_id)
	poll_id = 0
	loop.quit()

[3v1n0]

I think we should force the browser to use english language, otherwise all these are hard to test locally, so we should also set locale.setlocale(locale.LC_ALL, "C")