Right, but it seems to have one error handling mechanism in xorg-gtest, and another in xorg-integration-tests, especially with your commit message of "other clients will have to install their own error handlers"
On Wed, Nov 7, 2012 at 12:55 AM, Peter Hutterer <[email protected]>wrote: > On Wed, Nov 07, 2012 at 12:36:16AM -0500, Jasper St. Pierre wrote: > > I wonder if we should move the error trap system into xorg-gtest. > > yes, it would be useful have a unified error system for (both?) xorg-gtest > users. > > this is largely orthogonal to this commit though. the error trap system is > useful to trap specific errors that are expected, but not general errors > that may occur accidentally - which is what this patch addresses. > > Cheers, > Peter > > > On Tue, Nov 6, 2012 at 10:57 PM, Peter Hutterer < > [email protected]>wrote: > > > > > Xlib's default error handler prints the error and calls exit(1). Tests > that > > > accidentally trigger an error thus quit without cleaning up properly. > > > > > > Install a default error handler that prints the basic info and continue > > > with > > > the test. Clients that expect to trigger errors should set a custom > error > > > handler. > > > > > > Signed-off-by: Peter Hutterer <[email protected]> > > > --- > > > include/xorg/gtest/xorg-gtest-xserver.h | 13 ++++++ > > > src/xserver.cpp | 54 +++++++++++++++++++++++- > > > test/xserver-test.cpp | 73 > > > +++++++++++++++++++++++++++++++++ > > > 3 files changed, 139 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/xorg/gtest/xorg-gtest-xserver.h > > > b/include/xorg/gtest/xorg-gtest-xserver.h > > > index 8bf7996..11fc93d 100644 > > > --- a/include/xorg/gtest/xorg-gtest-xserver.h > > > +++ b/include/xorg/gtest/xorg-gtest-xserver.h > > > @@ -265,6 +265,19 @@ class XServer : public xorg::testing::Process { > > > */ > > > static void RegisterXIOErrorHandler(); > > > > > > + /** > > > + * Install a default XErrorHandler. That error handler will cause > a > > > test > > > + * failure if called. > > > + * > > > + * This function is called automatically by XServer::Start(). > Usually, > > > + * you will not need to call this function unless your test does > not > > > + * instantiate and Start() an XServer object. > > > + * > > > + * This function will only install a new error handler if the > > > currently > > > + * installed XErrorHandler is not the default handler used by > Xlib. > > > + */ > > > + static void RegisterXErrorHandler(); > > > + > > > private: > > > struct Private; > > > std::auto_ptr<Private> d_; > > > diff --git a/src/xserver.cpp b/src/xserver.cpp > > > index ad018a1..4faa8e9 100644 > > > --- a/src/xserver.cpp > > > +++ b/src/xserver.cpp > > > @@ -394,6 +394,40 @@ const std::string& > > > xorg::testing::XServer::GetVersion(void) { > > > return d_->version; > > > } > > > > > > +static int _x_error_handler(Display *dpy, XErrorEvent *err) > > > +{ > > > + std::stringstream error; > > > + switch(err->error_code) { > > > + case BadRequest: error << "BadRequest"; break; > > > + case BadValue: error << "BadValue"; break; > > > + case BadWindow: error << "BadWindow"; break; > > > + case BadPixmap: error << "BadPixmap"; break; > > > + case BadAtom: error << "BadAtom"; break; > > > + case BadCursor: error << "BadCursor"; break; > > > + case BadFont: error << "BadFont"; break; > > > + case BadMatch: error << "BadMatch"; break; > > > + case BadDrawable: error << "BadDrawable"; break; > > > + case BadAccess: error << "BadAccess"; break; > > > + case BadAlloc: error << "BadAlloc"; break; > > > + case BadColor: error << "BadColor"; break; > > > + case BadGC: error << "BadGC"; break; > > > + case BadIDChoice: error << "BadIDChoice"; break; > > > + case BadName: error << "BadName"; break; > > > + case BadLength: error << "BadLength"; break; > > > + case BadImplementation: error << "BadImplementation"; break; > > > + default: > > > + error << err->error_code; > > > + break; > > > + } > > > + > > > + ADD_FAILURE() << "XError received: " << error.str() << ", request " > << > > > + (int)err->request_code << "(" << (int)err->minor_code << "), > detail: " > > > + << err->resourceid << "\nThis error handler is likely to be > triggered > > > " > > > + "more than once.\nCheck the first error for the real error"; > > > + return 0; > > > +} > > > + > > > + > > > static int _x_io_error_handler(Display *dpy) _X_NORETURN; > > > static int _x_io_error_handler(Display *dpy) > > > { > > > @@ -409,6 +443,15 @@ void > xorg::testing::XServer::RegisterXIOErrorHandler() > > > XSetIOErrorHandler(old_handler); > > > } > > > > > > +void xorg::testing::XServer::RegisterXErrorHandler() > > > +{ > > > + XErrorHandler old_handler; > > > + old_handler = XSetErrorHandler(_x_error_handler); > > > + > > > + if (old_handler != _XDefaultError) > > > + XSetErrorHandler(old_handler); > > > +} > > > + > > > void xorg::testing::XServer::Start(const std::string &program) { > > > TestStartup(); > > > > > > @@ -464,7 +507,15 @@ void xorg::testing::XServer::Start(const > std::string > > > &program) { > > > args.push_back(it->second); > > > } > > > > > > - Process::Start(program.empty() ? d_->path_to_server : program, > args); > > > + std::string server_binary = program.empty() ? d_->path_to_server : > > > program; > > > + > > > + if (getenv("XORG_GTEST_XSERVER_USE_VALGRIND")) { > > > + args.insert(args.begin(), server_binary); > > > + server_binary = "valgrind"; > > > + args.insert(args.begin(), "--leak-check=full"); > > > + } > > > + > > > + Process::Start(server_binary, args); > > > /* noreturn */ > > > > > > } > > > @@ -494,6 +545,7 @@ void xorg::testing::XServer::Start(const > std::string > > > &program) { > > > signal(SIGUSR1 ,SIG_IGN); > > > > > > RegisterXIOErrorHandler(); > > > + RegisterXErrorHandler(); > > > } > > > > > > bool xorg::testing::XServer::Terminate(unsigned int timeout) { > > > diff --git a/test/xserver-test.cpp b/test/xserver-test.cpp > > > index cdf0bd9..32792e6 100644 > > > --- a/test/xserver-test.cpp > > > +++ b/test/xserver-test.cpp > > > @@ -6,6 +6,7 @@ > > > #include <stdexcept> > > > > > > #include <xorg/gtest/xorg-gtest.h> > > > +#include <gtest/gtest-spi.h> > > > #include <X11/extensions/XInput2.h> > > > > > > using namespace xorg::testing; > > > @@ -213,6 +214,78 @@ TEST(XServer, IOErrorException) > > > }, XIOError); > > > } > > > > > > +TEST(XServer, ErrorHandler) > > > +{ > > > + XORG_TESTCASE("Start a server, trigger a BadColor error and expect > a " > > > + "failure from the default error handler\n"); > > > + > > > + pid_t pid = fork(); > > > + > > > + if (pid == 0) { > > > + EXPECT_NONFATAL_FAILURE({ > > > + XServer server; > > > + server.SetOption("-logfile", LOGFILE_DIR > > > "/xorg-error-handler-test.log"); > > > + server.SetOption("-config", DUMMY_CONF_PATH); > > > + server.SetOption("-noreset", ""); > > > + server.Start(); > > > + ASSERT_EQ(server.GetState(), Process::RUNNING); > > > + ::Display *dpy = > XOpenDisplay(server.GetDisplayString().c_str()); > > > + ASSERT_TRUE(dpy != NULL); > > > + XColor color; > > > + XQueryColor(dpy, 0, &color); > > > + XSync(dpy, False); > > > + }, "BadColor"); > > > + exit(0); > > > + } > > > + > > > + /* if the Xlib default error handler triggers, child calls exit(1) > */ > > > + int status; > > > + ASSERT_EQ(waitpid(pid, &status, 0), pid); > > > + ASSERT_TRUE(WIFEXITED(status)); > > > + ASSERT_EQ(WEXITSTATUS(status), 0); > > > +} > > > + > > > +static bool error_handler_triggered = false; > > > + > > > +static int _test_error_handler(Display *dpy, XErrorEvent *error) { > > > + error_handler_triggered = true; > > > + if (error->error_code != BadColor) > > > + ADD_FAILURE() << "Error handler triggered with wrong error > code\n"; > > > + return 0; > > > +} > > > + > > > +TEST(XServer, NondefaultErrorHandler) > > > +{ > > > + XORG_TESTCASE("Set a custom error handler, start a server, trigger > a " > > > + "BadColor error and expect the custom error handler > to be > > > " > > > + "triggered\n"); > > > + > > > + pid_t pid = fork(); > > > + > > > + if (pid == 0) { > > > + XSetErrorHandler(_test_error_handler); > > > + > > > + XServer server; > > > + server.SetOption("-logfile", LOGFILE_DIR > > > "/xorg-error-handler-test.log"); > > > + server.SetOption("-config", DUMMY_CONF_PATH); > > > + server.SetOption("-noreset", ""); > > > + server.Start(); > > > + ASSERT_EQ(server.GetState(), Process::RUNNING); > > > + ::Display *dpy = XOpenDisplay(server.GetDisplayString().c_str()); > > > + ASSERT_TRUE(dpy != NULL); > > > + XColor color; > > > + XQueryColor(dpy, 0, &color); > > > + XSync(dpy, False); > > > + exit(error_handler_triggered ? 0 : 1); > > > + } > > > + > > > + /* if the Xlib default error handler triggers, child calls exit(1) > */ > > > + int status; > > > + ASSERT_EQ(waitpid(pid, &status, 0), pid); > > > + ASSERT_TRUE(WIFEXITED(status)); > > > + ASSERT_EQ(WEXITSTATUS(status), 0); > > > +} > > > + > > > TEST(XServer, KeepAlive) > > > { > > > XORG_TESTCASE("If XORG_GTEST_XSERVER_KEEPALIVE is set,\n" > > > -- > > > 1.7.11.7 > > > > > > _______________________________________________ > > > [email protected]: X.Org development > > > Archives: http://lists.x.org/archives/xorg-devel > > > Info: http://lists.x.org/mailman/listinfo/xorg-devel > > > > > > > > > > > -- > > Jasper > -- Jasper
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
