Bug 699 - TestCase::DoRun probably should not return a bool
TestCase::DoRun probably should not return a bool
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: core
pre-release
All All
: P5 normal
Assigned To: Craig Dowell
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-01 07:29 EDT by Mathieu Lacage
Modified: 2011-01-17 17:23 EST (History)
4 users (show)

See Also:


Attachments
Partial proposed patch for bug 699 (2.76 KB, patch)
2010-10-11 13:54 EDT, Mitch Watrous
Details | Diff
revised patch (5.96 KB, patch)
2011-01-12 01:24 EST, Tom Henderson
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mathieu Lacage 2009-10-01 07:29:51 EDT
It would be simpler for all testcases if we did not have to return something and merely used the status of TestCase::m_error as set by ReportFailure.
Comment 1 Mathieu Lacage 2009-11-24 04:37:38 EST
assigning to craig.
Comment 2 Craig Dowell 2010-01-04 14:48:28 EST
This will avoid the single line

  return GetErrorStatus ()

at the end of a test case implementation.  This is a conceptually simple thing to address but to do this would require touching many, many, many files all over the system.

If someone feels very strongly about it I can do this, but it really doesn't seem worth it.  I would absolutely not recommend trying to address this during a maintenance phase.
Comment 3 Lalith Suresh 2010-04-13 00:18:08 EDT
Is a patch still welcome for this bug? All we have to do is remove the 'return GetErrorStatus ()' lines right?
Comment 4 Mitch Watrous 2010-10-11 13:54:23 EDT
Created attachment 996 [details]
Partial proposed patch for bug 699 

Here is a patch from Tom Henderson that shows the changes that would be made to the TestCase class, the TestSuite class, and an example test suite:  src/test/sample-test-suite.cc.
Comment 5 Tom Henderson 2010-10-11 14:46:53 EDT
(In reply to comment #4)
> Created an attachment (id=996) [details]
> Partial proposed patch for bug 699 
> 
> Here is a patch from Tom Henderson that shows the changes that would be made to
> the TestCase class, the TestSuite class, and an example test suite: 
> src/test/sample-test-suite.cc.

To clarify, I think that the final patch may also involve streamlining src/core/test.{cc,h} since I don't think that there need to be as many variants of macros (e.g. the EXPECT_* versions) anymore, with one version using boolean returns and the other version not.

The main utility I see in this type of patch is actually in simplifying these macros and allowing single type of macro to be used both in callback functions and DoRun() methods.  But, I wonder whether that is the original motivation for filing this bug report, or something else.
Comment 6 Mathieu Lacage 2010-10-12 02:58:43 EDT
(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=996) [details] [details]
> > Partial proposed patch for bug 699 
> > 
> > Here is a patch from Tom Henderson that shows the changes that would be made to
> > the TestCase class, the TestSuite class, and an example test suite: 
> > src/test/sample-test-suite.cc.
> 
> To clarify, I think that the final patch may also involve streamlining
> src/core/test.{cc,h} since I don't think that there need to be as many variants
> of macros (e.g. the EXPECT_* versions) anymore, with one version using boolean
> returns and the other version not.
> 
> The main utility I see in this type of patch is actually in simplifying these
> macros and allowing single type of macro to be used both in callback functions
> and DoRun() methods.  But, I wonder whether that is the original motivation for
> filing this bug report, or something else.

The original motivation for this bug report is that a lot of test code returns an erronous value from these functions when they fail so that when the test fails, instead of merely being reported as a failing test, the test framework crashes on an NS_ASSERT. It's not fun to debug and I have had to do this more than once.
Comment 7 Mitch Watrous 2011-01-10 13:18:45 EST
All ns-3 users' test cases will have to be modified because this change is not backwards compatible.

Should we support both versions of DoRun():

  virtual void DoRun (void);

and

  virtual bool DoRun (void);
Comment 8 Mitch Watrous 2011-01-10 13:52:49 EST
I suspect that this change might introduce more bugs into ns-3 tests and ns-3 users' tests because it requires changing the logic of test cases that are currently all working.
Comment 9 Mathieu Lacage 2011-01-11 03:40:47 EST
let me repeat what I stated previously:
1) a lot of our test code is buggy and returns incorrect values from the function. These incorrect values are never seen until the test starts failing at which point it triggers an NS_ASSERT which does not improve debuggability of tests
2) we thus must fix our current buggy tests.
3) the easiest way to do this is to change the signature of this function and make sure that the build stops working for every test that has not been changed to the new signature. i.e., we have a compiler that can catch every instance automatically for us, and report it. 
4) The change is trivial to make in every piece of code: remove the return foo; statements and change them to "return".

So, my proposed plan is:
1) change signature
2) see build fail
3) fix build by removing un-needed values from return statements
4) profit the next time one of our tests fail

In general, changing API is not ok but in this case we have:
1) a good reason to do this: fix bugs that are hard to debug
2) a trivial way to catch every incorrect instance (compiler error messages)
3) a trivial way to fix the code that cannot be gotten wrong

as a maintainer of src/core/, I pre-approve the change but I have no time to perform it myself.
Comment 10 Mitch Watrous 2011-01-11 12:59:15 EST
(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=996) [details] [details]
> > Partial proposed patch for bug 699 
> > 
> > Here is a patch from Tom Henderson that shows the changes that would be made to
> > the TestCase class, the TestSuite class, and an example test suite: 
> > src/test/sample-test-suite.cc.
> 
> To clarify, I think that the final patch may also involve streamlining
> src/core/test.{cc,h} since I don't think that there need to be as many variants
> of macros (e.g. the EXPECT_* versions) anymore, with one version using boolean
> returns and the other version not.
> 
> The main utility I see in this type of patch is actually in simplifying these
> macros and allowing single type of macro to be used both in callback functions
> and DoRun() methods.  But, I wonder whether that is the original motivation for
> filing this bug report, or something else.

In your patch, at lines 682 of the original version of test.cc, the following changes were made to the function DoRun():

@@ -682,19 +675,17 @@ 
       //
       // Run the test case
       //
-      bool result = (*i)->Run ();
-      UpdateErrorStatus (result);
+      (*i)->Run ();

Should we keep those lines as they were because Run() still returns a bool value, and UpdateErrorStatus() needs to be called in order to set the error flag if an error occurred in Run()?
Comment 11 Tom Henderson 2011-01-12 01:24:32 EST
Created attachment 1023 [details]
revised patch

After reading this thread and the code more carefully, I think that this is what is being requested by Mathieu for the base classes.  

This patch would disallow users merely returning "true" from a test case to force an error to be recorded, which I think is the main way that this NS_ASSERT_MSG() is being hit.  If users use the macros or call ReportFailure(), then this assert shouldn't be hit.  However, if a user were to explicitly set SetErrorStatus or UpdateErrorStatus (true) to set the error status within TestCase::DoRun(), it would still be possible for the ASSERT to fire.
Comment 12 Mathieu Lacage 2011-01-12 02:19:33 EST
(In reply to comment #11)
> After reading this thread and the code more carefully, I think that this is
> what is being requested by Mathieu for the base classes.  

yes.
Comment 13 Mitch Watrous 2011-01-17 17:23:12 EST
Bug closed.

ns-3-dev changeset: 0783f42a364b