[PATCH xorg-gtest v2 4/8] xserver: move starting the process into the XServer object

[Peter Hutterer]

On Wed, Jul 11, 2012 at 11:57:50AM -0700, Chase Douglas wrote:
> On 07/10/2012 08:28 PM, Peter Hutterer wrote:
> >Changes from hardcoded vararg call to requiring the caller to set up options
> >for logfile, config, etc. beforehand. The display string is automatically
> >added to the command line.
> >
> >If no binary is given by the caller, the previously set binary path is used.
> >
> >Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> >---
> >Changes to v1:
> >- have one Start() call only handling with/without binary
> >- changes to accommodate for Process::Start(vector<std::string>) instead of
> >   previous const char *
> >
> >  include/xorg/gtest/xorg-gtest-xserver.h |    8 ++++++++
> >  src/environment.cpp                     |    7 +------
> >  src/xserver.cpp                         |   19 +++++++++++++++++++
> >  3 files changed, 28 insertions(+), 6 deletions(-)
> >
> >diff --git a/include/xorg/gtest/xorg-gtest-xserver.h b/include/xorg/gtest/xorg-gtest-xserver.h
> >index 1ba2a0f..0b7e7a0 100644
> >--- a/include/xorg/gtest/xorg-gtest-xserver.h
> >+++ b/include/xorg/gtest/xorg-gtest-xserver.h
> >@@ -47,6 +47,14 @@ class XServer : public xorg::testing::Process {
> >      XServer();
> >
> >      /**
> >+     * Start a new server. If no binary is given, the server started is the
> >+     * default compiled-in server binary.
> >+     *
> >+     * @param [in] program Path to the XServer binary
> >+     */
> >+    void Start(std::string program = std::string());
> 
> I'm still not sure we need the program argument. I would rather we
> stick to either setting the program path with SetServerPath(), or
> providing it in Start() as you have here. My preference would be to
> just drop SetServerPath(), but either approach is fine with me.

I think both will be useful, for different types of tests. SetServerPath is
good for tests where we start up the same (non-standard) server multiple
times:

setup():
    XServer s;
    s.SetServerPath("...");

test():
    s.Start();
    // test bits
    s.Terminate()

    s.Start();
    // test bits
    s.Terminate();

    ...

the program argument is useful for testing other DDXs:
    s.Start("Xorg");
    // test bits
    s.Terminate()

    s.Start("Xephyr");
    // test bits
    s.Terminate();
    ....

of course, none of them is impossible with the other approach, and I don't
actually have test cases for the second bit, though they're not far out.
it's just a convenience API.

> If you do leave this, pass in a const reference rather than a copy
> of the string. I see that you reuse the variable in the method, but
> using that as a reason to copy the string in would be leaking the
> implementation into the API.

done.

> >+
> >+    /**
> >       * Waits until this server is ready to take connections.
> >       */
> >      void WaitForConnections(void);
> >diff --git a/src/environment.cpp b/src/environment.cpp
> >index 7573b62..1521564 100644
> >--- a/src/environment.cpp
> >+++ b/src/environment.cpp
> >@@ -144,12 +144,7 @@ void xorg::testing::Environment::SetUp() {
> >    d_->server.SetDisplayNumber(d_->display);
> >    d_->server.SetOption("-logfile", d_->path_to_log_file);
> >    d_->server.SetOption("-config", d_->path_to_conf);
> >-  d_->server.Start(d_->path_to_server, d_->path_to_server.c_str(),
> >-                    display_string,
> >-                    "-logverbose", "10",
> >-                    "-logfile", d_->path_to_log_file.c_str(),
> >-                    "-config", d_->path_to_conf.c_str(),
> >-                    NULL);
> >+  d_->server.Start(d_->path_to_server);
> 
> Since you use d_->path_to_server as the default in Start(), there's
> no need to pass it in here.

the environment's path_to_server may differ to the built-in default from the
server, so we do need to pass this here.

Cheers,
  Peter

> >    d_->server.WaitForConnections();
> >
> >    Process::SetEnv("DISPLAY", display_string, true);
> >diff --git a/src/xserver.cpp b/src/xserver.cpp
> >index 62bd48f..2f9c364 100644
> >--- a/src/xserver.cpp
> >+++ b/src/xserver.cpp
> >@@ -253,6 +253,25 @@ void xorg::testing::XServer::WaitForConnections(void) {
> >    throw std::runtime_error("Unable to open connection to dummy X server");
> >  }
> >
> >+void xorg::testing::XServer::Start(std::string program) {
> >+  if (program.empty())
> >+    program = d_->path_to_server;
> >+
> >+  std::vector<std::string> args;
> >+  std::map<std::string, std::string>::iterator it;
> >+
> >+  args.push_back(program);
> >+  args.push_back(std::string(GetDisplayString()));
> >+
> >+  for (it = d_->options.begin(); it != d_->options.end(); it++) {
> >+    args.push_back(it->first);
> >+    if (!it->second.empty())
> >+      args.push_back(it->second);
> >+  }
> >+
> >+  Process::Start(program, args);
> >+}
> >+
> >  void xorg::testing::XServer::SetOption(std::string key, std::string value) {
> >    d_->options[key] = value;
> >  }
> >
> 
> With the fixes:
> 
> Reviewed-by: Chase Douglas <chase.douglas at canonical.com>