Merge lp://staging/~thomas-voss/xorg-gtest/Doxygen into lp://staging/~chasedouglas/xorg-gtest/fixes

It's a good start, but it's missing quite a bit:

* There's no Doxyfile or additions to the Makefiles to create and install the documentation

* Each object should have a description of what it is, at least enough so people familiar enough with gtest can figure it out. For example, Environment (the class) should have a brief line stating that it is a global gtest environment, and then a full description of what the class does.

I think some of these issues will become apparent when Doxygen is run and you view the generated documentation. I don't think the documentation here would be enough for people to understand how to use it without looking at the headers.

Other items:

* Environment::SetUp and TearDown documentation should make it clear that the functions should only be called by subclasses of Environment. We don't want people to think they need to call the functions themselves, even if they should know that already :).

* The variadic Process::Start documentation is missing documentation for the variadic arguments. Its documentation is also indented too far.

* Test::SetUp used to assert, but I pushed a fix so it would throw an exception instead. gtest catches exceptions so they are handled as failures, so this should not change the default functionality. However, throwing an exception is consistent with other routines and allows the user to catch it themselves. In summary, the documentation needs to be fixed to state exception throwing instead of failure asserting.

* Test::TearDown documentation says: "Closes the display if not NULL." What could be NULL? I think it just needs to say "Closes the display connection."

* I think the "brief" command clutters the documentation in the sources. I would rather use the Doxygen option that assumes the first paragraph of a comment is the brief section, and the rest is the detailed section.

* I prefer not using abbreviations for words. Non-native speakers may not be able to understand "feat." or "c'tor".

* Documenting a constructor as "Constructor" is a bit repetitive :). A better brief description would be something like "Constructs an object to provide a global X server dummy environment". This may repeat other documentation elsewhere, but that's ok.

Thanks!

> It's a good start, but it's missing quite a bit:
>
> * There's no Doxyfile or additions to the Makefiles to create and install the
> documentation
>

Done.

> * Each object should have a description of what it is, at least enough so
> people familiar enough with gtest can figure it out. For example, Environment
> (the class) should have a brief line stating that it is a global gtest
> environment, and then a full description of what the class does.
>

Done.

> I think some of these issues will become apparent when Doxygen is run and you
> view the generated documentation. I don't think the documentation here would
> be enough for people to understand how to use it without looking at the
> headers.
>
> Other items:
>
> * Environment::SetUp and TearDown documentation should make it clear that the
> functions should only be called by subclasses of Environment. We don't want
> people to think they need to call the functions themselves, even if they
> should know that already :).
>

Done. But I think we should change the methods' visibility to protected then.

> * The variadic Process::Start documentation is missing documentation for the
> variadic arguments. Its documentation is also indented too far.
>

Worked on this point.

> * Test::SetUp used to assert, but I pushed a fix so it would throw an
> exception instead. gtest catches exceptions so they are handled as failures,
> so this should not change the default functionality. However, throwing an
> exception is consistent with other routines and allows the user to catch it
> themselves. In summary, the documentation needs to be fixed to state exception
> throwing instead of failure asserting.
>

Fixed.

> * Test::TearDown documentation says: "Closes the display if not NULL." What
> could be NULL? I think it just needs to say "Closes the display connection."
>

Done.

> * I think the "brief" command clutters the documentation in the sources. I
> would rather use the Doxygen option that assumes the first paragraph of a
> comment is the brief section, and the rest is the detailed section.
>

Done.

> * I prefer not using abbreviations for words. Non-native speakers may not be
> able to understand "feat." or "c'tor".
>

Done.

> * Documenting a constructor as "Constructor" is a bit repetitive :). A better
> brief description would be something like "Constructs an object to provide a
> global X server dummy environment". This may repeat other documentation
> elsewhere, but that's ok.
>

Done.

> Thanks!

This is great! I especially like the code snippets in the detailed class documentation. With a few changes this will be ready for merging.

* Environment::SetUp() documentation says "Must not be called directly". This is only partly true. If you are setting up a subclass environment of xorg::testing::Environment, then you probably will be calling Environment::SetUp directly. This is how the utouch::testing::Environment works. The comment should be something like "Should only be called by subclasses. See Google test documentation for details."

* As I was reading the documentation, it occurred to me that we should use std::string for GetEnv and SetEnv. Can you update the code and documentation for this?

* xorg::testing::Test::Display() should be documented. It is protected so that subclassed testcases can access it. The TEST_F() macro creates a subclass of the test fixture, so both direct testcases of xorg::testing::Test and testcases of subclassed test fixtures (like utouch::frame::testing::Test) can access it.

* The documentation for xorg::testing::Test should be focused on it being a test fixture, not an object that checks for a running server (that's just one thing it does). For example, I would use the following as the brief description: "Google test fixture providing an Xlib connection to an X11 server." The detailed description should mention that the connection name is provided by the DISPLAY environment variable.

* There is the same issue with "must not be called directly" for the Test class.

* To go along with that, xorg::testing::Environment detailed documentation should state that the DISPLAY environment variable is updated to point to the dummy test server.

I just noticed that in the title of the main page, the word "Test" links to the Test class. There should be a way to escape this so the link does not occur.

One last thing: HIDE_SCOPE_NAMES should be set to NO in the Doxyfile so namespacing is clear in the documentation.

> This is great! I especially like the code snippets in the detailed class
> documentation. With a few changes this will be ready for merging.
>

Thanks :-)

> * Environment::SetUp() documentation says "Must not be called directly". This
> is only partly true. If you are setting up a subclass environment of
> xorg::testing::Environment, then you probably will be calling
> Environment::SetUp directly. This is how the utouch::testing::Environment
> works. The comment should be something like "Should only be called by
> subclasses. See Google test documentation for details."
>

Done.

> * As I was reading the documentation, it occurred to me that we should use
> std::string for GetEnv and SetEnv. Can you update the code and documentation
> for this?
>

Done.

> * xorg::testing::Test::Display() should be documented. It is protected so that
> subclassed testcases can access it. The TEST_F() macro creates a subclass of
> the test fixture, so both direct testcases of xorg::testing::Test and
> testcases of subclassed test fixtures (like utouch::frame::testing::Test) can
> access it.
>

Done.

> * The documentation for xorg::testing::Test should be focused on it being a
> test fixture, not an object that checks for a running server (that's just one
> thing it does). For example, I would use the following as the brief
> description: "Google test fixture providing an Xlib connection to an X11
> server." The detailed description should mention that the connection name is
> provided by the DISPLAY environment variable.
>

Done.

> * There is the same issue with "must not be called directly" for the Test
> class.
>

Done.

> * To go along with that, xorg::testing::Environment detailed documentation
> should state that the DISPLAY environment variable is updated to point to the
> dummy test server.

Done.

Introduced post conditions to methods to document changes to object states.

> I just noticed that in the title of the main page, the word "Test" links to
> the Test class. There should be a way to escape this so the link does not
> occur.

Done

> One last thing: HIDE_SCOPE_NAMES should be set to NO in the Doxyfile so
> namespacing is clear in the documentation.

Done.