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

[Chase Douglas]

On 07/11/2012 04:07 PM, Peter Hutterer wrote:
> 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.

I wondered if this wasn't the case :). Ok, sounds alright to me.

>> 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.

Ok, I got it.

-- Chase