Create autopkgtest

Bug #2060976 reported by Nathan Teodosio
This bug affects 1 person
Affects Status Importance Assigned to Milestone
freerdp2 (Ubuntu)
Fix Released
Nathan Teodosio

Bug Description

Ran successfully with autopkgtest -B . -- schroot noble-amd64.

Revision history for this message
Nathan Teodosio (nteodosio) wrote :
Revision history for this message
Paride Legovini (paride) wrote :

There are issues with the proposed diff.

1. the d/changelog entry is not descriptive of what the added test does. Please add something like (you'll know better):

 * d/tests: add basic connection test (LP: #....)

2. the debdiff adds d/t/ which is unused.

3. d/t/connect: better not specify shell options in the shebang, as that get lost if the script is invoked with `sh /path/to/script`. Use `set`.

4. d/t/connect: isNaturalNumber() returns 0 on the empty string, but we don't want that.

5. d/t/connect: this logic:

if isNaturalNumber "$port"; then
    exit 1

seems backwards to me: don't we want $port to be a natural number? FWIW I think it's enough to check that it's a nonempty string ([ -n "$port" ]).

6. xrdp.service has Why do we need to manually `sudo systemctl start xrdp`?

7. Can we make the `timeout 4s` longer? In general, can you add a comment on what we are testing more precisely?

Revision history for this message
Nathan Teodosio (nteodosio) wrote : Debdiff v2

Hi Paride, thanks for the review.

I've addressed your comments, except for number 6. For some reason this
failed when I tried it in the past. But if necessary I can try again and report
with the resulting error, as this I don't remember.

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

1/ d/changelog should refer to oracular now, not noble

+# XXX: Is there a better way to get the port number other than by parsing the
+# configuration file?
+port=$(sed -n '/^port=[0123456789]/{s/port=//p;q;}' /etc/xrdp/xrdp.ini)

Does the port change randomly, or is it always 3389 on a fresh install?

+if test -n "$port"; then
+ exit 1

I think you meant "test -z" instead.

+timeout 29s xvfb-run -l xfreerdp /v:localhost:"$port" /p: /u: /d:
+if test $? != 124; then
+ 2>&1 printf "%s\n" "Xfreerdp exited before the timeout, it has likely " \
+ "failed to connect. The test has therefore failed."
+ exit 2

Ok, this is a new type of test :). You are actually counting on the timeout command killing the binary for the test to succeed.

I'm not sure paride understood that before when he asked to increase this timeout. I think your original 4s was ok, because a failure to start should be quick.

I'm wondering if there is something else we could do here, but I don't think there are text-mode rdp clients out there ;) Do you know of something else that could exercise this rdp service at least a little bit more? Perhaps some auth negotiation? I'll leave that up to you to think about while you address the other points above, and it's not a blocker if there isn't anything else to add here that would be simple enough, yet give us a better idea of how well the package is working.

Changed in freerdp2 (Ubuntu):
assignee: nobody → Nathan Teodosio (nteodosio)
status: New → In Progress
Revision history for this message
Nathan Teodosio (nteodosio) wrote (last edit ): Re: [Bug 2060976] Re: Create autopkgtest

1. Fixed.

2. I'm hard coding it, we need to trust upstream to never change that value.

3. No longer applies.

> Do you know of
> something else that could exercise this rdp service at least a little
> bit more?

In this new patch I take a screen shot of the X framebuffer to check that it
really contains the XRDP log in page.

Test log:

Revision history for this message
Dave Jones (waveform) wrote :

I do like the addition of the compare test; it's a bit "magic" but the fact it's comparing against an image from the actual installation (/usr/share/xrdp/xrdp_logo.bmp) does demonstrate the operation of the package very nicely.

I did try and run the tests locally under autopkgtest but unfortunately they fail because the test is using "sudo" without declaring "needs-sudo" in the restrictions (without this, sudo isn't installed/configured for the test suite). Having patched that, the tests failed to run in a chroot due to the presence of needs-sudo (unsurprisingly), so I attempted with a full VM, but that failed with a badpkg error in imagemagick. So, this looks promising, but may need a bit more work to be certain it'll pass in production.

Revision history for this message
Nathan Teodosio (nteodosio) wrote :

For me, both the Launchpad runner and a local test with

  autopkgtest -B . -- null

succeed if I delete the

  sudo systemctl start xrdp

line. I must have misremembered when I wrote #3. Do you observe failure in any
of your environments if you delete that line?

Changed in freerdp2 (Ubuntu):
status: In Progress → New
Revision history for this message
Simon Chopin (schopin) wrote :

After a quick chmod +x, it works on my local machine with a lxd runner. However, I think it's best to just add the needs-sudo restriction since you're actually using it.

I've sponsored the upload (with the needs-sudo tag added).

Changed in freerdp2 (Ubuntu):
status: New → Fix Committed
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package freerdp2 - 2.11.5+dfsg1-1ubuntu1

freerdp2 (2.11.5+dfsg1-1ubuntu1) oracular; urgency=medium

  * d/tests: Create autopkgtest that checks whether the client
    can connect to a XRDP server via Freerdp2 and that the login
    screen really shows up (LP: #2060976).

 -- Nathan Pratta Teodosio <email address hidden> Mon, 27 May 2024 12:05:06 +0200

Changed in freerdp2 (Ubuntu):
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.