Code review comment for lp://staging/~thomas-voss/xorg-gtest/Doxygen

Revision history for this message
Chase Douglas (chasedouglas) wrote :

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.

review: Needs Fixing

« Back to merge proposal