[PATCH xorg-gtest v2 4/8] xserver: move starting the process into the XServer object
Peter Hutterer
peter.hutterer at who-t.net
Wed Jul 11 16:07:50 PDT 2012
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>
More information about the xorg-devel
mailing list