New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: run under Xvfb by default (if available) #2951

Merged
merged 1 commit into from Oct 13, 2017

Conversation

Projects
None yet
3 participants
@stapelberg
Member

stapelberg commented Sep 14, 2017

This shaves off two seconds of wall-clock time (10s → 8s).

@stapelberg stapelberg requested a review from Airblader Sep 14, 2017

@Airblader

This comment has been minimized.

Show comment
Hide comment
@Airblader

Airblader Sep 14, 2017

Member

What happens now if I explicitly wrap xvfb around the call?

Also the tedtsuite docs need to be adapted for this.

Member

Airblader commented Sep 14, 2017

What happens now if I explicitly wrap xvfb around the call?

Also the tedtsuite docs need to be adapted for this.

@stapelberg

This comment has been minimized.

Show comment
Hide comment
@stapelberg

stapelberg Sep 14, 2017

Member

Explicitly wrapping xvfb-run seems to result in a hang :).

I updated docs/testsuite. Let me know if I missed any other spot.

Member

stapelberg commented Sep 14, 2017

Explicitly wrapping xvfb-run seems to result in a hang :).

I updated docs/testsuite. Let me know if I missed any other spot.

}
if ($options{xvfb}) {
for (my $n = 99; $n < 120; $n++) {

This comment has been minimized.

@orestisf1993

orestisf1993 Sep 14, 2017

Member

I think you should check for "/tmp/.X$n-lock" and skip the current $n if it exists.

@orestisf1993

orestisf1993 Sep 14, 2017

Member

I think you should check for "/tmp/.X$n-lock" and skip the current $n if it exists.

This comment has been minimized.

@stapelberg

stapelberg Sep 14, 2017

Member

We could, but that’d amount to a TOCTOU issue. The way it is, Xvfb will error out when $n exists and we’ll increment and retry.

@stapelberg

stapelberg Sep 14, 2017

Member

We could, but that’d amount to a TOCTOU issue. The way it is, Xvfb will error out when $n exists and we’ll increment and retry.

This comment has been minimized.

@orestisf1993

orestisf1993 Sep 14, 2017

Member

A simple next if -e "/tmp/.X$n-lock";:

  1. Doesn't waste 2 seconds trying to start a xvfb instance that's going to fail
  2. Fixes the hang you described with xvfb-run. It happened again but later.
@orestisf1993

orestisf1993 Sep 14, 2017

Member

A simple next if -e "/tmp/.X$n-lock";:

  1. Doesn't waste 2 seconds trying to start a xvfb instance that's going to fail
  2. Fixes the hang you described with xvfb-run. It happened again but later.

This comment has been minimized.

@stapelberg

stapelberg Sep 14, 2017

Member
  1. We’re talking about the non-common case here, so we need to weigh the 2 seconds (on your machine — I don’t see such a delay at all) against the TOCTOU issue.
  2. How does the instruction fix the hang? That’s not intuitive to me.
@stapelberg

stapelberg Sep 14, 2017

Member
  1. We’re talking about the non-common case here, so we need to weigh the 2 seconds (on your machine — I don’t see such a delay at all) against the TOCTOU issue.
  2. How does the instruction fix the hang? That’s not intuitive to me.

This comment has been minimized.

@orestisf1993

orestisf1993 Sep 14, 2017

Member
  1. I don't understand the TOCTOU issue. If you make the change then worst case scenario is what the current code would do: xvfb fails and we continue with the next $n.
  2. Rerun and the hang happened again, so it was probably random. I think the issue here is that Xephyr gets the same display number with xvfb (xvfb-run defaults at :99) even though start_xserver is supposed to avoid that.

I also have a leftover process running at 100% from the sh line. I know my thoughts are spread out right now, I'll investigate once I get home.

@orestisf1993

orestisf1993 Sep 14, 2017

Member
  1. I don't understand the TOCTOU issue. If you make the change then worst case scenario is what the current code would do: xvfb fails and we continue with the next $n.
  2. Rerun and the hang happened again, so it was probably random. I think the issue here is that Xephyr gets the same display number with xvfb (xvfb-run defaults at :99) even though start_xserver is supposed to avoid that.

I also have a leftover process running at 100% from the sh line. I know my thoughts are spread out right now, I'll investigate once I get home.

This comment has been minimized.

@stapelberg

stapelberg Sep 16, 2017

Member

I think you’re right about the worst-case scenario being that we don’t use a display number which we could have used. However, I think it’s good practice to document trade-offs and shortcomings that one is aware of, so it’s not just a line of code — it’s a line of code with a comment explaining why a TOCTOU is okay. I don’t mind introducing it, but let’s do that in a separate PR.

@stapelberg

stapelberg Sep 16, 2017

Member

I think you’re right about the worst-case scenario being that we don’t use a display number which we could have used. However, I think it’s good practice to document trade-offs and shortcomings that one is aware of, so it’s not just a line of code — it’s a line of code with a comment explaining why a TOCTOU is okay. I don’t mind introducing it, but let’s do that in a separate PR.

This comment has been minimized.

@stapelberg

stapelberg Oct 13, 2017

Member

Following up on this: I decided to implement the check in the go.i3wm.org/i3 package, see https://github.com/i3/go-i3/blob/f2025a7c1730abd19d7a283083318ed90fcde613/common_test.go#L16 and https://github.com/i3/go-i3/blob/f2025a7c1730abd19d7a283083318ed90fcde613/validpid.go#L5

Note that doing it correctly means validating the pid contained within the file. If we don’t do it correctly, we risk unnecessarily incrementing the display numbers, possibly creating many unnecessary files.

This isn’t a reason not to implement the feature, but definitely a reason to punt it to a different PR :).

@stapelberg

stapelberg Oct 13, 2017

Member

Following up on this: I decided to implement the check in the go.i3wm.org/i3 package, see https://github.com/i3/go-i3/blob/f2025a7c1730abd19d7a283083318ed90fcde613/common_test.go#L16 and https://github.com/i3/go-i3/blob/f2025a7c1730abd19d7a283083318ed90fcde613/validpid.go#L5

Note that doing it correctly means validating the pid contained within the file. If we don’t do it correctly, we risk unnecessarily incrementing the display numbers, possibly creating many unnecessary files.

This isn’t a reason not to implement the feature, but definitely a reason to punt it to a different PR :).

@orestisf1993

This comment has been minimized.

Show comment
Hide comment
@orestisf1993

orestisf1993 Sep 14, 2017

Member

Explicitly wrapping xvfb-run seems to result in a hang :)

This should be fixed with #2952.

The hang happened with Xvfb on display ":100" because the X99 socket (used by xvfb-run by default) was sorted above X100. X100 was the socket used by the Xvfb instance started inside complete-run.pl because the for loop starts from 99, tries to open Xvfb on display 99, fails and increments $n to 100.

Member

orestisf1993 commented Sep 14, 2017

Explicitly wrapping xvfb-run seems to result in a hang :)

This should be fixed with #2952.

The hang happened with Xvfb on display ":100" because the X99 socket (used by xvfb-run by default) was sorted above X100. X100 was the socket used by the Xvfb instance started inside complete-run.pl because the for loop starts from 99, tries to open Xvfb on display 99, fails and increments $n to 100.

tests: run under Xvfb by default (if available)
This shaves off two seconds of wall-clock time (10s → 8s).
@orestisf1993

This comment has been minimized.

Show comment
Hide comment
@orestisf1993

orestisf1993 Sep 19, 2017

Member

Random thought: why run nested xvfb AND xephyr? Why not switch to plain xvfb and then try to use xephyr if it's not available?

A quick change passes all the tests except 533-randr15.t (which could probably be fixed), it's smaller and has approximately the same runtime.

diff --git a/testcases/lib/StartXServer.pm b/testcases/lib/StartXServer.pm
index 49976394..7a4217d9 100644
--- a/testcases/lib/StartXServer.pm
+++ b/testcases/lib/StartXServer.pm
@@ -104,8 +104,7 @@ sub start_xserver {
     my @sockets_waiting;
     for (1 .. $parallel) {
         my $socket = fork_xserver($keep_xserver_output, $displaynum,
-                'Xephyr', ":$displaynum", '-screen', '1280x800',
-                '-nolisten', 'tcp', '-name', "i3test");
+                'Xvfb', ":$displaynum", '-nolisten', 'tcp');
         push(@displays, ":$displaynum");
         push(@sockets_waiting, $socket);
         $displaynum++;
Member

orestisf1993 commented Sep 19, 2017

Random thought: why run nested xvfb AND xephyr? Why not switch to plain xvfb and then try to use xephyr if it's not available?

A quick change passes all the tests except 533-randr15.t (which could probably be fixed), it's smaller and has approximately the same runtime.

diff --git a/testcases/lib/StartXServer.pm b/testcases/lib/StartXServer.pm
index 49976394..7a4217d9 100644
--- a/testcases/lib/StartXServer.pm
+++ b/testcases/lib/StartXServer.pm
@@ -104,8 +104,7 @@ sub start_xserver {
     my @sockets_waiting;
     for (1 .. $parallel) {
         my $socket = fork_xserver($keep_xserver_output, $displaynum,
-                'Xephyr', ":$displaynum", '-screen', '1280x800',
-                '-nolisten', 'tcp', '-name', "i3test");
+                'Xvfb', ":$displaynum", '-nolisten', 'tcp');
         push(@displays, ":$displaynum");
         push(@sockets_waiting, $socket);
         $displaynum++;
@stapelberg

This comment has been minimized.

Show comment
Hide comment
@stapelberg

stapelberg Sep 23, 2017

Member

Random thought: why run nested xvfb AND xephyr? Why not switch to plain xvfb and then try to use xephyr if it's not available?

Good point! I just tried this, but it turns out that using Xvfb directly is significantly slower on my system (6 seconds instead of 4 seconds). I’m not sure yet why that is.

Member

stapelberg commented Sep 23, 2017

Random thought: why run nested xvfb AND xephyr? Why not switch to plain xvfb and then try to use xephyr if it's not available?

Good point! I just tried this, but it turns out that using Xvfb directly is significantly slower on my system (6 seconds instead of 4 seconds). I’m not sure yet why that is.

@orestisf1993

This comment has been minimized.

Show comment
Hide comment
@orestisf1993

orestisf1993 Sep 23, 2017

Member

I think xvfb starting up has some significant overhead. Try xvfb-run echo hello. Xephyr seems to be pretty fast instead.

Member

orestisf1993 commented Sep 23, 2017

I think xvfb starting up has some significant overhead. Try xvfb-run echo hello. Xephyr seems to be pretty fast instead.

@stapelberg

This comment has been minimized.

Show comment
Hide comment
@stapelberg

stapelberg Oct 13, 2017

Member

Let’s go ahead with this as-is, we can always improve it later on.

Member

stapelberg commented Oct 13, 2017

Let’s go ahead with this as-is, we can always improve it later on.

@stapelberg stapelberg merged commit 44a6efb into i3:next Oct 13, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stapelberg stapelberg deleted the stapelberg:xvfb branch Oct 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment