After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 721624 - Regression in GTest framework reorders existing test cases
Regression in GTest framework reorders existing test cases
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2014-01-06 11:18 UTC by Tristan Van Berkom
Modified: 2014-01-08 11:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Document g_test_run() order better, and how it changed (3.32 KB, patch)
2014-01-07 16:20 UTC, Dan Winship
committed Details | Review

Description Tristan Van Berkom 2014-01-06 11:18:17 UTC
This happened around 6 months ago iirc.

Basically, the straight forward / easy use case of using GTest is to write
a main() function where you list test cases:

  g_test_add ("/Something/Probably/Lowlevel/Comes/First"...);
  g_test_add ("/Some/Other/Low/Level/Tests"...);
  ...
  /* Now we've tested our subsystems, make sure complex combinations work */
  g_test_add ("/Atomic/Completion/Of/Some/Complex/Task"...);
  ...
  return g_test_run ();

Many many test suites are written this way, but GTest suddenly decided that
it was ok to decide all on it's own that running the higher level test:

      "/Atomic/Completion/Of/Some/Complex/Task"

Before running the series of lower level tests which were clearly intended
by the test case author to run first.

This means that when the highlevel "/Atomic/Completion/Of/Some/Complex/Task"
fails, we have no clue *why* it failed, because we don't know which of the
preceding tests failed.

I'm calling this a regression because GTest stopped respecting the meaningful
order of test cases which already exist in years and years worth of test cases,
this means that if GTest is not fixed to respect the order which test cases
are defined, we effectively force test cause authors to modify their test cases
in order to get the desired order again.
Comment 1 Dan Winship 2014-01-06 15:19:07 UTC
(Actually, in the example you gave, the tests will run in order.)

The behavior that changed is when you do:

  g_test_add_func ("/alpha/lowlevel", alpha_lowlevel);
  g_test_add_func ("/beta/lowlevel", beta_lowlevel);
  g_test_add_func ("/alpha/complicated-atomic-test", alpha_atomic);

glib used to run them in that order. But that was actually a bug. According to the documentation, the tests are grouped into suites, so "/alpha/complicated-atomic-test" should run before "/beta/lowlevel" because it's part of the "alpha" suite, which runs before the "beta" suite.

Tests are still run in the order you define them within a suite, and suites are run in the order that their first test is defined in. So all you have to do to fix the order is change the test paths to reflect the grouping you want:

  g_test_add_func ("/lowlevel/alpha", alpha_lowlevel);
  g_test_add_func ("/lowlevel/beta", beta_lowlevel);
  g_test_add_func ("/complicated-atomic-tests/alpha", alpha_atomic);


Leaving the bug open because we should probably document this better.
Comment 2 Tristan Van Berkom 2014-01-07 06:34:56 UTC
(In reply to comment #1)
> (Actually, in the example you gave, the tests will run in order.)
> 
> The behavior that changed is when you do:
> 
>   g_test_add_func ("/alpha/lowlevel", alpha_lowlevel);
>   g_test_add_func ("/beta/lowlevel", beta_lowlevel);
>   g_test_add_func ("/alpha/complicated-atomic-test", alpha_atomic);
> 
> glib used to run them in that order. But that was actually a bug. According to
> the documentation, the tests are grouped into suites, so
> "/alpha/complicated-atomic-test" should run before "/beta/lowlevel" because
> it's part of the "alpha" suite, which runs before the "beta" suite.
> 

Dan, thanks for the input, this is something constructive that I can digest.

And it IS indeed complicated, this is a sort of example of what I have which
is bothering me - I have something ordered like this:

  * Birthing tests
  /Foo/Birthing/TestLeprechauns
  /Foo/Birthing/TestBunnies
  /Foo/Birthing/TestBunniesGiveBirthToLeprechauns

  * Dinner time tests
  /Foo/Dinner/TestLeprechauns
  /Foo/Dinner/TestBunnies
  /Foo/Dinner/TestLeprechaunsEatingBunniesForDinner

  * Various other, more complex tests. These don't feel right to go into
  * any logical 'group', so each one is normally a standalone complex test
  * which is unrelated to any other sub-path
  /Foo/TestDeliciousnessOfCuteKittens
  /Foo/TestHungerOfBunniesAndLeprechauns
  /Foo/TestBunniesAndLeprechaunsSharingATastyCuteKitten

This is how I would normally go about defining my tests, there are usually
grouped tests at the beginning, and a few more hefty tests added to the end
(so you'll understand my alarm when GTest started to place all of my most
complex tests "at the beginning" just because GTest judged that shorter
test paths should come first).

I've tested your explanation and found that if I name the last tests with
a prefix of '/Foo/Complex' instead of just '/Foo', then they do preserve
the order in which they were declared - I can work with this (it will mean
I have to change how some of my tests are declared) - but I agree this should
be well documented as it's an easy trap to fall into.

Thanks again, Dan, for the clarification.
Comment 3 Dan Winship 2014-01-07 16:20:51 UTC
Created attachment 265542 [details] [review]
Document g_test_run() order better, and how it changed
Comment 4 Emmanuele Bassi (:ebassi) 2014-01-07 16:25:57 UTC
Review of attachment 265542 [details] [review]:

looks good to me.
Comment 5 Matthias Clasen 2014-01-07 16:36:42 UTC
Review of attachment 265542 [details] [review]:

It would be good to add a general recommendation that unit tests are ideally independent of each other, so order should not matter. Before diving into the details of the ordering guarantees we make... feel free to push with that addition
Comment 6 Dan Winship 2014-01-07 17:35:20 UTC
pushed with an added note that the results of individual tests should
never depend on what order the tests run in
Comment 7 Tristan Van Berkom 2014-01-08 11:45:57 UTC
This seems to still be broken, sadly :(

From the following text from the applied patch:

+ * The tests and sub-suites within each suite are run in the order in
+ * which they are defined. However, note that prior to GLib 2.36,
+ * there was a bug in the <literal>g_test_add_*</literal> functions
+ * which caused them to create multiple suites with the same name,
+ * meaning that if you created tests "/a/one", "/b/one", and "/a/two"
+ * in that order, they would get run in that order (since g_test_run()
+ * would run the first "/a" suite, then the "/b" suite, then the
+ * second "/a" suite). As of 2.36, this bug is fixed, and adding the
+ * tests in that order would result in a running order of "/a/one",
+ * "/a/two", "/b/one". In cases where this causes problems, you should
+ * change the test paths to group tests by suite in a way that will
+ * result in the desired running order. Eg, "/one/a", "/one/b",
+ * "/two/a".

  "The tests and sub-suites within each suite are run in the order
   in which they are defined."

The above is still incorrect and I'm still getting undefined behavior,
here is the order in which I define my suite:

 * Object destruction tests *
 /StateMachine/Destroy/Idle
 /StateMachine/Destroy/Timeout
 /StateMachine/Destroy/GSignal
 /StateMachine/Destroy/NativeSignal
 /StateMachine/Destroy/Transient

 * Transition priority tests *
 /StateMachine/Priority/Scope
 /StateMachine/Priority/Condition
 /StateMachine/Priority/OrthogonalCondition

 * Timeout test *
 /StateMachine/Timeout/Reset: OK

 * State deactivation tests *
 /StateMachine/Deactivate/State: OK
 /StateMachine/Deactivate/Substate: OK
 /StateMachine/Deactivate/IntermediateState: OK
 /StateMachine/Deactivate/OrthogonalSibling: OK

 * Finally, more complex tests *
 /StateMachine/Complex/ActiveStateCondition: OK
 /StateMachine/Complex/SynchronizedTransitions: OK
 /StateMachine/Complex/AtomicTransition: OK
 /StateMachine/Complex/EnterOrthogonalSubstate: OK
 /StateMachine/Complex/Activate/IntermediateState: OK

And here is the order in which they run:

 /StateMachine/Destroy/Idle: OK
 /StateMachine/Destroy/Timeout: OK
 /StateMachine/Destroy/GSignal: OK
 /StateMachine/Destroy/NativeSignal: OK
 /StateMachine/Destroy/Transient: OK
 /StateMachine/Priority/Scope: OK
 /StateMachine/Priority/Condition: OK
 /StateMachine/Priority/OrthogonalCondition: OK
 /StateMachine/Complex/ActiveStateCondition: OK
 /StateMachine/Complex/SynchronizedTransitions: OK
 /StateMachine/Complex/AtomicTransition: OK
 /StateMachine/Complex/EnterOrthogonalSubstate: OK
 /StateMachine/Complex/Activate/IntermediateState: OK
 /StateMachine/Timeout/Reset: OK
 /StateMachine/Deactivate/State: OK
 /StateMachine/Deactivate/Substate: OK
 /StateMachine/Deactivate/IntermediateState: OK
 /StateMachine/Deactivate/OrthogonalSibling: OK

Again, even when declaring everything nicely into sub suites, GTest
makes the broken assumption that it's OK to reorder suites, so the
 "/StateMachine/Complex"
tests which should absolutely run last, are running before other tests
which are testing subsystems for which the "Complex" functionality depends on.
Comment 8 Tristan Van Berkom 2014-01-08 11:49:14 UTC
Gah - Sorry this is my bad.

I had actually defined one of the "Complex" tests before the "Timeout" test.

Sorry for the noise, when I define the "Complex" tests at the bottom they
do indeed run in the correct order.