GNOME Bugzilla – Bug 692125
Support TAP as GTest output format
Last modified: 2015-11-25 11:34:31 UTC
Created attachment 233932 [details] [review] add start/stop suite log messages Some initial patches towards producing TAP output, which would let us use other test harnesses than gtester.
Created attachment 233933 [details] [review] Add a GTestResult enum
Created attachment 233934 [details] [review] allow to mark tests as skipped or incomplete
Created attachment 233935 [details] [review] support tap
Created attachment 233936 [details] [review] optionally make assertions nonfatal
Review of attachment 233932 [details] [review]: One question, otherwise looks good to me. ::: glib/gtestutils.h @@ +297,3 @@ + G_TEST_LOG_MESSAGE, /* s:blurb */ + G_TEST_START_SUITE, + G_TEST_STOP_SUITE What's our compatibility story with respect to extending enums? For this one it probably doesn't matter since I doubt anyone is doing a switch () on it. But if we added a new one to say GFileType, that seems like it could break apps.
Review of attachment 233933 [details] [review]: OK
Review of attachment 233934 [details] [review]: Some convoluted code here, I'll trust you on this one; just two minor bits: ::: glib/gtestutils.c @@ +1513,3 @@ + * If not called from inside a test, this function does nothing. + * + * Since: 2.30 2.36 @@ +1536,3 @@ + * If not called from inside a test, this function does nothing. + * + * Since: 2.30 2.36
Review of attachment 233935 [details] [review]: Cool, I like TAP.
(In reply to comment #5) > What's our compatibility story with respect to extending enums? For this one > it probably doesn't matter since I doubt anyone is doing a switch () on it. > > But if we added a new one to say GFileType, that seems like it could break > apps. we usually operate under the assumption that adding enumeration values is not an ABI break. to my recollection, and a quick check on the commit history, we've been doing that for years and years - with or without the explicit 'this enumeration may be extended at any later date' warning in the documentation blurb. if you're switch()ing on an enumeration, you should have a 'default' rule to catch eventual new enumeration values; alternatively, use compiler warnings to catch that when rebuilding against a newer version of the libraries.
Review of attachment 233936 [details] [review]: But this doesn't stop the test case from continuing execution, right? I think porting tests is going to be a lot harder than just #define G_NONFATAL_ASSERT, because a lot of the GLib tests would immediately deference a NULL pointer or the like if they continued after an assertion. Painful as it may be, I think the sane path forward is to add a new test signature that uses GError.
Review of attachment 233935 [details] [review]: looks cool to me, and I like TAP as well. :-)
Review of attachment 233934 [details] [review]: looks okay to me
Review of attachment 233936 [details] [review]: I agree with Colin: most of the tests written with GTest will end up in an inconsistent state after a failed g_assert(), and most likely segfault anyway. if the g_test_fail() immediately returned after the failure of a unit so that the test suite would continue running, it would be okay. another one way to avoid the assertion would be to run each test unit in a separate process, and catch the assertion failure, though this would change a lot of the internals of GTest.
(In reply to comment #13) > Review of attachment 233936 [details] [review]: > > another one way to avoid the assertion would be to run each test unit in a > separate process, and catch the assertion failure, though this would change a > lot of the internals of GTest. Yeah, I think this is what we need to do.
I think(In reply to comment #5) > Review of attachment 233932 [details] [review]: > > One question, otherwise looks good to me. > > ::: glib/gtestutils.h > @@ +297,3 @@ > + G_TEST_LOG_MESSAGE, /* s:blurb */ > + G_TEST_START_SUITE, > + G_TEST_STOP_SUITE > > What's our compatibility story with respect to extending enums? For this one > it probably doesn't matter since I doubt anyone is doing a switch () on it. GTest is an unfortunate example of 'guts spilled out' api. This stuff is really all GTest internals that shouldn't have been in the header anywasy. That being said, we can easily do without the start suite/end suite additions here, they are just used to generate some comments in tap output. But doing so was useful in that it make me find that g_test_add_vtable was totally busted.
(In reply to comment #13) > I agree with Colin: most of the tests written with GTest will end up in an > inconsistent state after a failed g_assert(), and most likely segfault anyway. > if the g_test_fail() immediately returned after the failure of a unit so that > the test suite would continue running, it would be okay. > > another one way to avoid the assertion would be to run each test unit in a > separate process, and catch the assertion failure, though this would change a > lot of the internals of GTest. So, that last patch was really mainly for discussion - thanks for providing that discussion ! I was aware of the fact that returning is needed, just a little scared of changing the semantics of asserts in that way. But maybe it is ok, since asserts are expected to be fatal. There's still the problem that adding a return will make asserts break the build in non-void functions... One other idea I had was to tie the behaviour change to a runtime condition, say g_test_is_initialized().
(In reply to comment #10) > But this doesn't stop the test case from continuing execution, right? I think > porting tests is going to be a lot harder than just #define G_NONFATAL_ASSERT, > because a lot of the GLib tests would immediately deference a NULL pointer or > the like if they continued after an assertion. Sure, you can't just define G_NONFATAL_ASSERT in existing test programs and expect them to work. But you could write test programs that did work with it. The main reason I've never ported libsoup to use gtester is that the libsoup tests are all written to not crash on errors (where possible), and I like the fact that if something breaks, I can see *all* of the test failures rather than only the first one. (In reply to comment #10) > Painful as it may be, I think the sane path forward is to add a new test > signature that uses GError. (In reply to comment #13) > another way to avoid the assertion would be to run each test unit in a > separate process, and catch the assertion failure, though this would change a > lot of the internals of GTest. It's nice to be able to report multiple errors in a single test case too though. Eg, one gtest antipattern that pops up a lot is: success = my_op (arg, arg, &error); g_assert (success); g_assert_no_error (error); If my_op fails, you're only going to see "assertion failed: success" rather than the much more useful message "got error 'file not found'". (So put the two assertions in the other order!) But anyway, if the test kept running after the first assertion, then it wouldn't matter what order you put them in. And lots of tests check multiple things that are semi-independent of each other. Eg, in mainloop.c there are lots of "check that source_a did what we expected, then check that source_b did what we expected, etc". It's useful to be able to still see if source b did the right thing even if source a failed. (Yes, in some cases the errors will be correlated, and you'll get redundant messages. But sometimes that's a useful data point too...)
Comment on attachment 233936 [details] [review] optionally make assertions nonfatal This needs to deal with g_warning(), etc, as well; currently they'd still be fatal errors, right? Having a g_test_set_nonfatal_errors() or whatever might be better than having a #define
Created attachment 235079 [details] [review] add start/stop suite log messages
Created attachment 235080 [details] [review] update the protocol test
Created attachment 235081 [details] [review] add a GTestResult enum
Created attachment 235082 [details] [review] add functions to mark test as skipped or incomplete
Created attachment 235083 [details] [review] add a function to check test status
Created attachment 235084 [details] [review] support TAP as an output format
Created attachment 235085 [details] [review] make g_assert and g_assert_not_reached use the same entry point
Created attachment 235086 [details] [review] add g_assert_true
Created attachment 235087 [details] [review] optionally make assertions nonfatal
Created attachment 235088 [details] [review] update docs
Created attachment 235089 [details] [review] start using tap
Here is another attempt; this time there is a g_test_set_nonfatal_assertions(). The last patch shows how converting glib tests to the automake tap driver and test harness looks.
Review of attachment 235079 [details] [review]: looks okay.
Review of attachment 235080 [details] [review]: dunno, maybe merge with the previous commit? also, it would be nice to mention TAP.
Review of attachment 235081 [details] [review]: looks okay.
Review of attachment 235082 [details] [review]: looks okay.
Review of attachment 235083 [details] [review]: looks okay.
Review of attachment 235084 [details] [review]: looks good.
Review of attachment 235085 [details] [review]: looks good.
Review of attachment 235087 [details] [review]: ::: glib/gtestutils.c @@ +1644,3 @@ +g_test_set_nonfatal_assertions (void) +{ + g_return_if_fail (g_test_config_vars->test_initialized); I would probably g_error(), here.
Review of attachment 235086 [details] [review]: prior art in other test suites is assert_true, assert_false, assert_null. should we add the others as well?
Pushed with the requested additions of g_assert_false, g_assert_null
Did this ever make it into the master branch and a release? I don't see a --tap output option in recent versions of gtester and I can not see any of these commits in git master.
Yes, you can use TAP with the GTest API — using the TAP support inside Automake, and without using the gtester helper (which is somewhat deprecated). See the glib-tap.mk file: https://git.gnome.org/browse/glib/tree/glib-tap.mk I also wrote a quick guide on Stack Overflow on how to enable TAP for projects using GTest: http://stackoverflow.com/questions/19958861/how-to-properly-set-up-glib-testing-framework-with-autotools/19975406#19975406 Various projects have been migrated to using TAP instead of gtester; the main issue remaining is hitting an assertion inside a unit will terminate the whole suite, instead of skipping to the following unit; see bug 722648.