RESOLVED FIXED 237708
[GTK] Add a unit test to check the remote inspector HTTP server
https://bugs.webkit.org/show_bug.cgi?id=237708
Summary [GTK] Add a unit test to check the remote inspector HTTP server
Carlos Garcia Campos
Reported 2022-03-10 05:24:02 PST
Check that target list page is served as expected like we do for remote inspector server.
Attachments
Patch (16.26 KB, patch)
2022-03-10 05:28 PST, Carlos Garcia Campos
no flags
Carlos Garcia Campos
Comment 1 2022-03-10 05:28:35 PST
Michael Catanzaro
Comment 2 2022-03-10 06:10:31 PST
Comment on attachment 454340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454340&action=review > Tools/TestWebKitAPI/Tests/WebKitGtk/TestInspectorServer.cpp:67 > + g_printerr("Failed to connect to inspector server"); This one actually does need to end in \n > Tools/TestWebKitAPI/Tests/WebKitGtk/TestInspectorServer.cpp:105 > + // Wait until server is ready. > + unsigned timeoutID = g_timeout_add(25000, [](gpointer userData) { > + g_main_loop_quit(static_cast<GMainLoop*>(userData)); > + return G_SOURCE_REMOVE; > + }, m_mainLoop); Hmm, what is the purpose of this? You are trying to fail the test if it takes more than 2.5 seconds for the server to start? I would either use a way larger timeout -- say 30 seconds, to match the standard D-Bus timeout -- or else remove it altogether and let the test time out if it's broken. Otherwise, I would be afraid the test could be flaky if the system is under heavy load. I see you've already taken care to retry the connection every 100ms and quit immediately if it succeeds, which is good. > Tools/TestWebKitAPI/Tests/WebKitGtk/TestInspectorServer.cpp:228 > + // We install a handler to ensure that we kill the child process > + // if the parent dies because of whatever the reason is. > + signal(SIGABRT, +[](int) { Since you're only catching SIGABRT, I would say "if the parent dies because of an assertion failure." Alternative: you might try prctl(PR_SET_PDEATHSIG) and see if that works, then the child should die even if the parent dies to something other than SIGABRT. But I've seen PR_SET_PDEATHSIG mysteriously fail, so maybe make sure it works for you if you decide to try this.
Carlos Garcia Campos
Comment 3 2022-03-11 01:31:35 PST
Comment on attachment 454340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454340&action=review >> Tools/TestWebKitAPI/Tests/WebKitGtk/TestInspectorServer.cpp:67 >> + g_printerr("Failed to connect to inspector server"); > > This one actually does need to end in \n Ok. >> Tools/TestWebKitAPI/Tests/WebKitGtk/TestInspectorServer.cpp:105 >> + }, m_mainLoop); > > Hmm, what is the purpose of this? You are trying to fail the test if it takes more than 2.5 seconds for the server to start? I would either use a way larger timeout -- say 30 seconds, to match the standard D-Bus timeout -- or else remove it altogether and let the test time out if it's broken. Otherwise, I would be afraid the test could be flaky if the system is under heavy load. > > I see you've already taken care to retry the connection every 100ms and quit immediately if it succeeds, which is good. I think this was needed before because the server was started in beforeAll() and the test runner handles the timeout per test. Now that the server is started/stopped as part of the test setup/teardown we can probably remove this and let the runner handle the timeout. >> Tools/TestWebKitAPI/Tests/WebKitGtk/TestInspectorServer.cpp:228 >> + signal(SIGABRT, +[](int) { > > Since you're only catching SIGABRT, I would say "if the parent dies because of an assertion failure." > > Alternative: you might try prctl(PR_SET_PDEATHSIG) and see if that works, then the child should die even if the parent dies to something other than SIGABRT. But I've seen PR_SET_PDEATHSIG mysteriously fail, so maybe make sure it works for you if you decide to try this. Yes, I think the idea here was to kill the server when the test fails, not when it crashes.
Carlos Garcia Campos
Comment 4 2022-03-11 02:36:49 PST
Note You need to log in before you can comment on or make changes to this bug.