[PATCH xorg-gtest 05/16] xserver: move testing startup to the XServer object

[Chase Douglas]

On 07/05/2012 04:27 PM, Peter Hutterer wrote:
> On Thu, Jul 05, 2012 at 01:46:42PM -0700, Chase Douglas wrote:
>> On 07/03/2012 10:34 PM, Peter Hutterer wrote:
>>> On Tue, Jul 03, 2012 at 10:30:28AM -0700, Chase Douglas wrote:
>>>> On 07/02/2012 11:44 PM, Peter Hutterer wrote:
>>>>> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
>>>>
>>>> It makes sense to move the startup to the XServer object, but the
>>>> environment should then contain an XServer object and start it. The
>>>> point of the environment is that it starts up an xserver
>>>> automatically.
>>>>
>>>> The xorg-gtest user is free to not use the environment. But when
>>>> they use the environment, it should start a server.
>>>
>>> It still does, it's just cut off from the Environment::SetUp() hunk though.
>>> see below
>>>
>>>>> ---
>>>>>   include/xorg/gtest/xorg-gtest-xserver.h |    2 ++
>>>>>   src/environment.cpp                     |   35 ---------------------------
>>>>>   src/xserver.cpp                         |   40 +++++++++++++++++++++++++++++++
>>>>>   3 files changed, 42 insertions(+), 35 deletions(-)
>>>>>
>>>>> diff --git a/include/xorg/gtest/xorg-gtest-xserver.h b/include/xorg/gtest/xorg-gtest-xserver.h
>>>>> index 5c5ce99..52a2fd0 100644
>>>>> --- a/include/xorg/gtest/xorg-gtest-xserver.h
>>>>> +++ b/include/xorg/gtest/xorg-gtest-xserver.h
>>>>> @@ -129,6 +129,8 @@ class XServer : public xorg::testing::Process {
>>>>>       XServer(const XServer&);
>>>>>       XServer& operator=(const XServer&);
>>>>>
>>>>> +    void TestStartup(void);
>>>>> +
>>>>>   };
>>>>>   } // namespace testing
>>>>>   } // namespace xorg
>>>>> diff --git a/src/environment.cpp b/src/environment.cpp
>>>>> index 7ed23b3..69972a4 100644
>>>>> --- a/src/environment.cpp
>>>>> +++ b/src/environment.cpp
>>>>> @@ -106,41 +106,6 @@ void xorg::testing::Environment::SetUp() {
>>>>>     static char display_string[6];
>>>>>     snprintf(display_string, 6, ":%d", d_->display);
>>>>>
>>>>> -  Display* test_display = XOpenDisplay(display_string);
>>>>> -  if (test_display) {
>>>>> -    XCloseDisplay(test_display);
>>>>> -    std::string message;
>>>>> -    message += "A server is already running on ";
>>>>> -    message += display_string;
>>>>> -    message += ".";
>>>>> -    throw std::runtime_error(message);
>>>>> -  }
>>>>> -
>>>>> -  /* The Xorg server won't start unless the log file and the old log file are
>>>>> -   * writable. */
>>>>> -  std::ofstream log_test;
>>>>> -  log_test.open(d_->path_to_log_file.c_str(), std::ofstream::out);
>>>>> -  log_test.close();
>>>>> -  if (log_test.fail()) {
>>>>> -    std::string message;
>>>>> -    message += "X.org server log file ";
>>>>> -    message += d_->path_to_log_file;
>>>>> -    message += " is not writable.";
>>>>> -    throw std::runtime_error(message);
>>>>> -  }
>>>>> -
>>>>> -  std::string old_log_file = d_->path_to_log_file.c_str();
>>>>> -  old_log_file += ".old";
>>>>> -  log_test.open(old_log_file.c_str(), std::ofstream::out);
>>>>> -  log_test.close();
>>>>> -  if (log_test.fail()) {
>>>>> -    std::string message;
>>>>> -    message += "X.org old server log file ";
>>>>> -    message += old_log_file;
>>>>> -    message += " is not writable.";
>>>>> -    throw std::runtime_error(message);
>>>>> -  }
>>>>> -
>>>>>     d_->server.SetDisplayNumber(d_->display);
>>>>>     d_->server.SetLogfilePath(d_->path_to_log_file);
>>>>>     d_->server.SetConfigPath(d_->path_to_conf);
>>>
>>> is followed by
>>>      d_->server.Start(d_->path_to_server);
>>>      d_->server.WaitForConnections();
>>>
>>> So nothing should change over the previous code. I haven't modified the
>>> xi2.cpp code in the server which uses this environment and it still works.
>>
>> Ahh, right, the code still starts the server, but shouldn't we be
>> calling the server TestStartup() method before? As it is in this
>> patch, we've removed a bunch of code, moved it to a method of a
>> different object, and haven't called the method to ensure the same
>> code is executed. Unless I'm misreading again :).
>
> hard to see from the patch I guess, but the new sequence is
>      Environment::SetUp()
>         ...
>         XServer::Start()
>              XServer::TestStartup()
>              Process::Start()

Ok, that looks good. I guess some of the context is just getting strewn 
across a couple different patches.

-- Chase