Re: [PATCH xorg-gtest v2 1/8] Add a new class representing the X server

Wed, 11 Jul 2012 17:42:02 -0700 

On 07/11/2012 03:16 PM, Peter Hutterer wrote:

On Wed, Jul 11, 2012 at 11:35:47AM -0700, Chase Douglas wrote:

On 07/10/2012 08:28 PM, Peter Hutterer wrote:

This class is a Process, so it's a drop-in replacement for the current
Environment, but it provides a few methods to talk to the server being
started.

SetDisplayNumber() is called, but currently unused by the server.

Signed-off-by: Peter Hutterer <[email protected]>
---
Changes to v1:
- squashed a few minor patches in so this class is a bit more complete out
of the box.

  configure.ac                            |    2 +-
  include/Makefile.am                     |    1 +
  include/xorg/gtest/xorg-gtest-xserver.h |  126 ++++++++++++++++++++
  include/xorg/gtest/xorg-gtest.h         |    1 +
  src/Makefile.am                         |    1 +
  src/environment.cpp                     |   22 ++--
  src/xorg-gtest-all.cpp                  |    1 +
  src/xserver.cpp                         |  196 +++++++++++++++++++++++++++++++
  8 files changed, 339 insertions(+), 11 deletions(-)
  create mode 100644 include/xorg/gtest/xorg-gtest-xserver.h
  create mode 100644 src/xserver.cpp

diff --git a/configure.ac b/configure.ac
index ea29c50..d9b3072 100644
--- a/configure.ac
+++ b/configure.ac
@@ -25,7 +25,7 @@ XORG_DEFAULT_OPTIONS
  XORG_ENABLE_INTEGRATION_TESTS([yes])
  XORG_WITH_DOXYGEN

-PKG_CHECK_MODULES(X11, x11)
+PKG_CHECK_MODULES(X11, x11 xi)

  # Check for Google Test
  CHECK_GTEST
diff --git a/include/Makefile.am b/include/Makefile.am
index 9ff5f2a..4a6ce03 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -34,6 +34,7 @@ nobase_include_HEADERS = \
        xorg/gtest/xorg-gtest-environment.h \
        xorg/gtest/xorg-gtest-process.h \
        xorg/gtest/xorg-gtest-test.h \
+       xorg/gtest/xorg-gtest-xserver.h \
        xorg/gtest/evemu/xorg-gtest-device.h \
        xorg/gtest/xorg-gtest.h \
        $(compat_headers)
diff --git a/include/xorg/gtest/xorg-gtest-xserver.h 
b/include/xorg/gtest/xorg-gtest-xserver.h
new file mode 100644
index 0000000..56dacf6
--- /dev/null
+++ b/include/xorg/gtest/xorg-gtest-xserver.h
@@ -0,0 +1,126 @@
+/*******************************************************************************
+ *
+ * X testing environment - Google Test helper class to communicate with the
+ * server
+ *
+ * Copyright (C) 2012 Red Hat, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN 
THE
+ * SOFTWARE.
+ *
+ 
******************************************************************************/
+
+
+#ifndef XORG_GTEST_XSERVER_H
+#define XORG_GTEST_XSERVER_H
+
+#include <gtest/gtest.h>
+#include <xorg/gtest/xorg-gtest.h>
+#include <X11/Xlib.h>
+
+namespace xorg {
+namespace testing {
+
+/**
+ * @class XServer xorg-gtest-xserver.h xorg/gtest/xorg-gtest-xserver.h


Minor niggle: doxygen is smart enough to know that we're documenting
a class without having to tell it. I like leaving out the "@class"
since it just clutters things up.


we can do that change but since we'd have to update all other header files
too (they all use @class atm) it's better to do this in one go, stating the
reason for doing so.
Oh, right. I didn't realize all the existing classes were documented like that. You're right that we should be uniform, and this isn't a big enough deal to worry about patching anyway :). 


[...]


+#include <sys/types.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#include <algorithm>
+#include <cerrno>
+#include <csignal>
+#include <cstdio>
+#include <cstdlib>
+#include <cstring>
+#include <stdexcept>
+#include <vector>
+
+#include <X11/extensions/XInput2.h>
+
+struct xorg::testing::XServer::Private {
+  unsigned int display_number;
+  std::string display_string;
+};
+
+xorg::testing::XServer::XServer() : d_(new Private) {
+  SetDisplayNumber(d_->display_number);


If I'm reading this correctly, d_ will be allocated with unset
values. d_->display_number will potentially be any integer. Then, we
call SetDisplayNumber() with the same random integer.

We should be calling SetDisplayNumber(DEFAULT_DISPLAY).


oops, rebase fallout, at some point I had display_number initialised. I've
added
   d_->display_number = DEFAULT_DISPLAY;
before the SetDisplayNumber() call.


Ok

-- Chase
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Reply via email to