Loading ...
Sorry, an error occurred while loading the content.
 

"Hard to test" (Was RE: [XP] [refactoring] More on: Introduce Parameter Object )

Expand Messages
  • Laurent Bossavit
    ... Aw... Can t be all that hard. Try : public class Concurrency extends TestSuite { public Concurrency(String name) { super(name); } public static TestSuite
    Message 1 of 15 , Mar 1, 2001
      Kari writes :

      > No. I don't have a test. Bugs like this are difficult to find with tests.

      Aw... Can't be all that hard. Try :

      public class Concurrency extends TestSuite {

      public Concurrency(String name) {
      super(name);
      }

      public static TestSuite suite() {
      TestSuite suite = new TestSuite("");
      suite.addTest(new Concurrency("Concurrency test"));
      return suite;
      }

      class query_comparer {

      private String m_q1, m_q2;
      private Object m_r1, m_r2;

      public void set_queries(String q1, String q2) {
      m_r1 = new Object();
      m_r2 = new Object();
      m_q1 = q1;
      m_q2 = q2;
      }

      public String show_differences() {
      return m_q1.equals(m_q2) ? "" : "yes";
      }

      }

      public void run(TestResult testResult) {
      testResult.startTest(this);
      final TestResult result = testResult;
      final query_comparer qc = new query_comparer();
      final String set1Query1 = "a";
      final String set1Query2 = "a";
      final String set2Query1 = "a";
      final String set2Query2 = "b";
      final Test theTest = this;
      Thread firstThread = new Thread() {
      public void run() {
      qc.set_queries(set1Query1,set1Query2);
      try {Thread.sleep(1000);} catch (Exception e) {}
      if (!qc.show_differences().equals("")) {
      result.addFailure(theTest,new AssertionFailedError
      ("Expected no difference."));
      }
      }
      };
      Thread secondThread = new Thread() {
      public void run() {
      qc.set_queries(set2Query1,set2Query2);
      if (qc.show_differences().equals("")) {
      result.addFailure(theTest,new AssertionFailedError
      ("Expected a difference."));
      }
      }
      };
      firstThread.start();
      secondThread.start();
      try {Thread.sleep(2000);} catch (Exception e) {}
      testResult.endTest(this);
      }

      }

      In plain English, what I suspect you have experienced is a
      concurrency bug, resulting from the "error" of not making thread-
      safe a class of which one given instance may be used in more than
      one thread.

      I may of course be wrong, and maybe the test above is not a test
      for the kind of bug you are thinking of. If so, feel free to slap my
      wrist in public. ;)

      On the other hand, I do maintain that the above test exposes a bug
      if class query_comparer is assumed (consciously or not) to be
      thread safe.

      The conclusion I would like to draw is the following.

      Concurrency issues are a category of bug-inducing issues which
      everyone, including persons such as Ron Jeffries whose opinion on
      the matter I am inclined to respect, believes to be "hard to test".

      My (admittedly limited) concrete experience with concurrency
      issues, and some (admittedly theoretical) work I have done on this
      lead me to disagree with this conclusion; IMHO concurrency
      issues are not intrinsically harder to test than other classes of
      bugs, although they might be just a little more difficult to think
      about.

      The main problem seems to be that of ensuring deterministic
      behaviour so you don't have to resort to "Monte Carlo testing".
      Though I am not capable of doing it myself, I believe it should be
      possible to extend existing testing frameworks so as to generalize
      the above method for ensuring deterministic behaviour.

      (N.B. The above code has been compiled and run with the
      expected results.)

      ========================================
      A successful technology creates problems
      that only it can solve.
      ========================================
      Laurent Bossavit - Software Architect
      >>> laurent.bossavit@... <<<
      >>> 06 68 15 11 44 <<<
      >> ICQ#39281367 <<
      Agence Bless http://www.agencebless.com/
      ========================================
    • kari.hoijarvi@vaisala.com
      Concurrency is a good guess, but this time it s not what I have in my mind. It might be right with real ADO, but my pseudo ADO obviously always works. Well, my
      Message 2 of 15 , Mar 1, 2001
        Concurrency is a good guess, but this time it's not what I have in my mind.
        It might be right with real ADO, but my pseudo ADO obviously always works.

        Well, my work day is ending here in Helsinki, so I'll write my thoughts
        about it tomorrow.

        Kari

        P.S. What you wrote about concurrency is impressive. I have been working
        with threads for years and you would be an instant hire.

        -----Original Message-----
        From: Laurent Bossavit [mailto:laurent.bossavit@...]

        In plain English, what I suspect you have experienced is a
        concurrency bug, resulting from the "error" of not making thread-
        safe a class of which one given instance may be used in more than
        one thread.

        I may of course be wrong, and maybe the test above is not a test
        for the kind of bug you are thinking of. If so, feel free to slap my
        wrist in public. ;)
      • Ron Jeffries
        ... Outstanding! Way to go! Ronald E Jeffries http://www.XProgramming.com http://www.objectmentor.com
        Message 3 of 15 , Mar 1, 2001
          At 01:50 PM 3/1/2001 +0100, it seemed like Laurent Bossavit wrote:
          >(N.B. The above code has been compiled and run with the
          >expected results.)

          Outstanding! Way to go!

          Ronald E Jeffries
          http://www.XProgramming.com
          http://www.objectmentor.com
        • Laurent Bossavit
          ... Damn, wrong again. I suppose the reason I guessed wrong is the usual suspect - eagerness to talk - and show off - overriding willingness to listen. (I do
          Message 4 of 15 , Mar 1, 2001
            > Concurrency is a good guess, but this time it's not what I have in
            > my mind.

            Damn, wrong again.

            I suppose the reason I guessed wrong is the usual suspect -
            eagerness to talk - and show off - overriding willingness to listen. (I
            do appreciate the kudos very much.)

            If I'd paid more attention to your words I probably would have
            focused on "mutable member variables". Let's see - I think I know
            for sure that String objects are all immutable; you can't even have a
            mutable subclass as String is final. So you must be talking about
            RecordSet.

            Is the bug related to the contract of whatever class "db"
            implements ? Specifically, does the "db" instance retain a
            reference to the RecordSet that they return, and maybe alter that
            RecordSet in subsequent calls to Execute ?

            If I'm correct *this* time, my next question would be "Isn't this hard
            to test only because the bug is in the class of db, not in your own
            code ?". This would be a rather restricted meaning of "hard to test",
            but there might be appropriate solutions to this specific kind of
            problem too.


            ========================================
            The best way to predict the future is to
            invent it.
            ========================================
            Laurent Bossavit - Software Architect
            >>> laurent.bossavit@... <<<
            >>> 06 68 15 11 44 <<<
            >> ICQ#39281367 <<
            Agence Bless http://www.agencebless.com/
            ========================================
          • dmitry@yahoo.com
            ... Java and ... Are you referring to the possibility that m_q1 and m_q2 could be initialized incorrectly if the values of q1 and q2 are modified by the
            Message 5 of 15 , Mar 1, 2001
              --- In extremeprogramming@y..., kari.hoijarvi@v... wrote:
              > Here is an example of mutable member variables, written in pseudo
              Java and
              > using pseudo ADO for db access.
              >
              > What's wrong with it? Can you spot the bug?
              >
              > class query_comparer {
              > private String m_q1, m_q2:
              > private Recordset m_r1, m_r2;
              >
              > public set_queries(String q1, String q2) throws DBException
              > {
              > m_r1 = db.Execute(q1);
              > m_r2 = db.Execute(q2);
              > m_q1 = q1;
              > m_q2 = q2;
              > }
              >
              > public show_differences(......
              > };

              Are you referring to the possibility that m_q1 and m_q2 could be
              initialized incorrectly if the values of q1 and q2 are modified by
              the respective db.Execute() calls? Just a guess...

              Dmitry
            • Roger Lipscombe
              ... I suspect it s more a problem with the fact that the client code can call show_differences before they ve called set_queries. They ll get unexpected
              Message 6 of 15 , Mar 1, 2001
                > --- In extremeprogramming@y..., kari.hoijarvi@v... wrote:
                > > Here is an example of mutable member variables, written in pseudo
                > Java and
                > > using pseudo ADO for db access.
                > >
                > > What's wrong with it? Can you spot the bug?
                > >
                > > class query_comparer {
                > > private String m_q1, m_q2:
                > > private Recordset m_r1, m_r2;
                > >
                > > public set_queries(String q1, String q2) throws DBException
                > > {
                > > m_r1 = db.Execute(q1);
                > > m_r2 = db.Execute(q2);
                > > m_q1 = q1;
                > > m_q2 = q2;
                > > }
                > >
                > > public show_differences(......
                > > };
                >
                > Are you referring to the possibility that m_q1 and m_q2 could be
                > initialized incorrectly if the values of q1 and q2 are modified by
                > the respective db.Execute() calls? Just a guess...

                I suspect it's more a problem with the fact that the client code can call
                show_differences before they've called set_queries. They'll get unexpected
                results.

                The correct code - assuming I'm right - (in C++, since I can only read, not
                write Java)...

                using std::string;

                class query_composer {
                const string m_q1, m_q2;
                Recordset m_r1, m_r2;
                public:
                query_composer(string q1, string q2) : m_q1(q1), m_q2(q2)
                {
                m_r1 = db.Execute(q1);
                m_r2 = db.Execute(q2);
                }

                void show_differences(.......
                };

                Now the items are just as private as before, and we've forced the user to
                initialise the object correctly.
              • Eric Bennett
                ... Well there s a suspect colon there, but that s too easy. ... Is it that the object state is not well defined if an exception is thrown? Most suspect part
                Message 7 of 15 , Mar 1, 2001
                  On Thu, 1 Mar 2001 kari.hoijarvi@... wrote:

                  > What's wrong with it? Can you spot the bug?
                  >
                  > class query_comparer {
                  > private String m_q1, m_q2:

                  Well there's a suspect colon there, but that's too easy.

                  > private Recordset m_r1, m_r2;
                  >
                  > public set_queries(String q1, String q2) throws DBException
                  > {
                  > m_r1 = db.Execute(q1);
                  > m_r2 = db.Execute(q2);
                  > m_q1 = q1;
                  > m_q2 = q2;
                  > }
                  >
                  > public show_differences(......
                  > };

                  Is it that the object state is not well defined if an exception is thrown?
                  Most suspect part is that the m_q* members are set only if the query
                  executions succeed. But whether that's a problem or not depends on
                  query_composers' responsibility for handling errors and on who does the
                  cleanup.

                  - Eric B.

                  --
                  "Pasteurized From Concentrate"
                • Robert Watkins
                  ... That s easy... it s because Java (the language of the examples) passes everything by value. Consider the following code: public void doSomething(int x, int
                  Message 8 of 15 , Mar 1, 2001
                    Barbini Uberot wrote:

                    > I need more detail on Introduce Parameter Object Refactor ;))
                    > This is one of not-trivial refactoring more useful to me.
                    >
                    > Anyway I disagree about a rule:
                    > why pass all data field in constructor and declare them final?

                    That's easy... it's because Java (the language of the examples) passes
                    everything by value.

                    Consider the following code:
                    public void doSomething(int x, int y) {
                    x = 5; // Deliberately silly step taken.
                    y = 3;
                    }

                    int a = 2;
                    int b = 4;
                    doSomething(a, b);
                    System.out.println(a + ", " + b); // prints "2, 4" not "5, 3".


                    Now, a simple use of Introduce Parameter Object would have a change in
                    behaviour. Consider:
                    public void doSomething(ParameterObject param) {
                    param.x = 5;
                    param.y = 3;
                    }

                    int a = 2;
                    int b = 4;
                    ParameterObject myParam = new ParameterObject(a, b);
                    doSomething(param);
                    System.out.println(param.x + ", " + param.y); //prints "5, 3", not "2, 4"

                    Martin Fowler recommends that you use the refactor
                    RemoveAssignmentToParameters, by declaring the parameters final first. This
                    gets the language to enforce the expected behaviour.

                    If the parameters are final, then when you introduce the ParameterObject,
                    they should be final there.

                    Also remember that ParameterObjects really should be tossed away after use.
                    The calling code in the example should have been:

                    int a = 2;
                    int b = 4;
                    doSomething(new ParameterObject(a, b));
                    System.out.println(a, b);


                    > My parameter objects tends to develop some behavior and an
                    > autonomous live,
                    > so now I just start with default value in constructor and
                    > accessor for every
                    > data changeable.

                    If your object evolves, then it can become more powerful. Introduce
                    Parameter Object doesn't address this.

                    > Moreover creating and destoing an object all the times produces a
                    > considerable overload if I have to call these methods many
                    > times for second.
                    > It's not a problem of performance but of memory fragmentation.

                    That is an issue. Introduce Parameter Object can cause problems with this.
                    In this case, I would suggest making the parameter object mutable, but don't
                    introduce the side effects. In either case, this is beyond the scope of
                    Introduce Parameter Object.

                    Finally, remember that not all refactorings are applicable all of the time.

                    Robert.

                    --
                    "Duct tape is like the Force: it has a light side, a dark side,
                    and it holds the universe together"
                    Robert Watkins robertdw@... robertw@...


                    [Non-text portions of this message have been removed]
                  • kari.hoijarvi@vaisala.com
                    So here s my answer. Congratulations for Eric Bennett and Robert Sartin for getting the right answer. Honorable mention to Laurent Bossavit for noticing thread
                    Message 9 of 15 , Mar 2, 2001
                      So here's my answer. Congratulations for Eric Bennett and Robert Sartin for
                      getting the right answer.

                      Honorable mention to Laurent Bossavit for noticing thread safety: ADO and
                      ODBC are designed thread-safe, but all the ODBC drivers are not!

                      The test to expose bug:

                      1. Call set_queries with "select * from table1" and "select * from table2"

                      after the call:
                      m_q1 == "select * from table1"
                      m_q2 == "select * from table2"
                      m_r1 == (data from table1)
                      m_r2 == (data from table2)

                      2. lock table4 by administrator, so that it cannot be queried

                      3. Call set_queries with "select * from table3" and "select * from table4"

                      after the call the state is inconsistent, since m_r2 = db.Execute(q2)
                      throws.
                      m_q1 == "select * from table1"
                      m_q2 == "select * from table2"
                      m_r1 == (data from table3) // here's the inconsistency
                      m_r2 == (data from table2)

                      Finding this bug with testing is hard. You have to run the method twice and
                      make the second query to throw.

                      Now when it comes to exception safety, there are 4 guarantees:

                      1. transactional: system makes a rollback to original states
                      2. basic: objects cannot be used but can be deleted after an exception
                      3. nothrow: exception cannot occur
                      4. no quarantee: objects cannot even be deleted after an exception

                      query_comparer satisfies only the basic quarantee. Sometimes the basic
                      quarantee is enough, sometimes it's not. Here are my suggestions for fixing
                      it:

                      1. transactional quarantee: Turn set_queries into a constructor and make the
                      class immutable. If mutability is needed, make the class transactional:

                      class mutable_query_comparer {
                      private query_comparer m_iqc;

                      public set_queries(String q1, String q2) throws DBException
                      {
                      m_iqc = new query_comparer(q1, q2);
                      }

                      public show_differences(......
                      };

                      Now, if the immutable_query_comparer throws, m_iqc does not change.

                      One reason I love functional programming, immutable classes and storing
                      mutable data in DB is, that transactional quarantee is easy to do.

                      2. basic quarantee: improve communication. Make sure, that show_differences
                      does not work if the state is inconsistent:

                      public set_queries(String q1, String q2) throws DBException
                      {
                      m_r1 = null;
                      m_r2 = null;
                      m_q1 = null;
                      m_q2 = null;
                      m_r1 = db.Execute(q1);
                      m_r2 = db.Execute(q2);
                      m_q1 = q1;
                      m_q2 = q2;
                      }

                      public show_differences(.....
                      {
                      // check validity of every member here.

                      Obviously if you code this way you better have a valid reason to do it.

                      3. nothrow:

                      Dijkstra does not like exceptions, since they interfere badly with
                      correctness proof. He prefers to program "sufficiently large computer" that
                      never fails because of resource exhaustion. Since it's impossible to build
                      it, he runs his programs in the "hopefully sufficiently large computer" that
                      aborts instead of throwing. This is a very valid approach to many types of
                      programs, for example compilers.

                      4. no guarantee:

                      This is the de facto quality standard in the C++ world. For example take a
                      look into CSimpleMap in ATL: It cannot be used if the constructors of Key or
                      Val classes can throw. It cannot be used with anything else either, since
                      there is a resource leak bug in Remove method. It's excellent teaching
                      material if you need to teach inspection techniques for someone.

                      With Java, writing no quarantee classes requires creative uses of finalize.

                      I have held 4 public classes during last year and nobody got this right.
                      Exception safety is hard and with C++ it's exceptionally hard.

                      Kari

                      > Here is an example of mutable member variables, written in pseudo
                      Java and
                      > using pseudo ADO for db access.
                      >
                      > What's wrong with it? Can you spot the bug?
                      >
                      > class query_comparer {
                      > private String m_q1, m_q2:
                      > private Recordset m_r1, m_r2;
                      >
                      > public set_queries(String q1, String q2) throws DBException
                      > {
                      > m_r1 = db.Execute(q1);
                      > m_r2 = db.Execute(q2);
                      > m_q1 = q1;
                      > m_q2 = q2;
                      > }
                      >
                      > public show_differences(......
                      > };
                    • Laurent Bossavit
                      ... Nice ! The test doesn t sound hard to write once you know how; it does appear that it might be hard to figure out that s what you need. But please read on.
                      Message 10 of 15 , Mar 2, 2001
                        > The test to expose bug:
                        >
                        > 1. Call set_queries with "select * from table1" and "select * from table2"
                        > 2. lock table4 by administrator, so that it cannot be queried
                        > 3. Call set_queries with "select * from table3" and "select * from table4"

                        Nice ! The test doesn't sound hard to write once you know how; it
                        does appear that it might be hard to figure out that's what you
                        need. But please read on.

                        > Finding this bug with testing is hard. You have to run the method twice and
                        > make the second query to throw.

                        "Aha" moment : we don't mean the same thing by "hard to test".

                        In understood you to mean "hard to reproduce in test code the
                        behaviour that leads to the bug, so a UT can reliably expose it".
                        The problem of testing concurrency issues is my canonical
                        example of that.

                        I now understand that you meant "hard to ensure that test-first
                        alone, as opposed to an awareness of reliability issues and the
                        dangers of ignoring them, will result in reliable code".

                        I had a second "Aha" moment looking at your code; I realized I
                        might have seen this problem before and used essentially the
                        same tactics. Tell me if this looks familiar. Excerpts below, before
                        and after :

                        * Before

                        public class DispatcherServlet extends HttpServlet implements SingleThreadModel {

                        // Why is this failing even with SingleThreadModel !?

                        String remoteUser = "";
                        String remotePass = "";

                        HttpServletRequest request;
                        HttpServletResponse response;

                        public void service(HttpServletRequest request, HttpServletResponse response)
                        throws ServletException, java.io.IOException
                        {
                        this.request = request;
                        this.response = response;
                        // ...
                        }

                        private void getCredentials() {
                        try {
                        remoteUser = getUserFromHeader();
                        remotePass = getPasswordFromHeader();
                        }
                        catch (Exception e) {}
                        }

                        * After a first fix : use the "basic guarantee" tactic :

                        private void getCredentials() {
                        remoteUser = "";
                        remotePass = "";
                        try {
                        // ...etc...

                        * After a second fix : Introduce Parameter Object

                        public class DispatcherServlet extends HttpServlet {

                        private class RequestData {
                        public String remoteUser = "";
                        public String remotePass = "";

                        public HttpServletRequest request;
                        public HttpServletResponse response;
                        }

                        public void service(HttpServletRequest request, HttpServletResponse response)
                        throws ServletException, IOException
                        {
                        RequestData myData = new RequestData();
                        // ...
                        }

                        private void getCredentials(RequestData myData) {
                        // ...as before...



                        ========================================
                        Bless n'a pas de modele : son ambition
                        est d'en devenir un.
                        ========================================
                        Laurent Bossavit - Software Architect
                        >>> laurent.bossavit@... <<<
                        >>> 06 68 15 11 44 <<<
                        >> ICQ#39281367 <<
                        Agence Bless http://www.agencebless.com/
                        ========================================
                      • kari.hoijarvi@vaisala.com
                        ... From: Laurent Bossavit [mailto:laurent.bossavit@agencebless.com] ... I have had code like this too. I consider this very hard to test. ... Yes. Exception
                        Message 11 of 15 , Mar 2, 2001
                          -----Original Message-----
                          From: Laurent Bossavit [mailto:laurent.bossavit@...]

                          > "Aha" moment : we don't mean the same thing by "hard to test".

                          > In understood you to mean "hard to reproduce in test code the
                          > behaviour that leads to the bug, so a UT can reliably expose it".
                          > The problem of testing concurrency issues is my canonical
                          > example of that.

                          I have had code like this too. I consider this "very hard to test."

                          > I now understand that you meant "hard to ensure that test-first
                          > alone, as opposed to an awareness of reliability issues and the
                          > dangers of ignoring them, will result in reliable code".

                          Yes. Exception safety is hard to ensure with tests. Not impossible, but
                          hard. I had a fellow who worked with a big application exception safety for
                          a month. His strategy was to allow one more memory allocation each time he
                          started the smoke test. He didn't get very far during that month.

                          On the other hand, proper reviews with statistical testing gives a good idea
                          about exception safety. I have our exception raising and HRESULT checking
                          calling DebugBreak every time an error occurs. At that point, I go to the
                          debugger and walk thru the live exception handling, line by line, focusing
                          in code correctness. After this inspection, I remove the DebugBreak call.

                          So far 44 calls, 1.2% overall have been inspected without a found defect.
                          Simple statistics gives 89% confidentiality level that 95% of the exception
                          handling is correct.

                          Statistical testing of this kind is easy. Writing automated tests for all
                          those 3500 possible exceptions would be an expensive way to find bugs.

                          > I had a second "Aha" moment looking at your code; I realized I
                          > might have seen this problem before and used essentially the
                          > same tactics. Tell me if this looks familiar. Excerpts below, before
                          > and after :

                          Your code is pretty much a replica of my example. Thanks for giving a real
                          world examnple for me! I'll save this discussion for my future lectures.

                          This is a typical inconsistent state bug. Inspection is the cheapest tool to
                          find these. I think couple of my students had this same feeling of code
                          smell: they protested against the code saying it was stupid, but could not
                          tell exactly why.


                          Kari
                        • Laurent Bossavit
                          ... Rings true. So we might conclude the following. Some classes of problems are hard to test in the sense that test- first will not completely ensure
                          Message 12 of 15 , Mar 2, 2001
                            > This is a typical inconsistent state bug. Inspection is the
                            > cheapest tool to find these.

                            Rings true. So we might conclude the following.

                            Some classes of problems are "hard to test" in the sense that test-
                            first will not completely ensure reliability.

                            Some classes of problems are "hard to test" in the sense of it
                            being hard to automate the testing.

                            It seems to us that the answer to the first is to perform inspection -
                            in an XP team, pair programming has this role, which can be seen
                            as supplementing the reliability benefits of test-first.

                            It seems to us that the answer to the last is to work hard on the
                            tools that will make automation possible, and eventually less hard -
                            because outside of issues requiring human judgement tests that
                            can be done manually can be automated.


                            ========================================
                            We aim to make simple things simple and
                            complex things possible.
                            ========================================
                            Laurent Bossavit - Software Architect
                            >>> laurent.bossavit@... <<<
                            >>> 06 68 15 11 44 <<<
                            >> ICQ#39281367 <<
                            Agence Bless http://www.agencebless.com/
                            ========================================
                          • Robert Sartin
                            ... The challenge with this sort of problem isn t so much that is hard to test as that if you think of the test, it s because you don t have the bug . These
                            Message 13 of 15 , Mar 2, 2001
                              --- Laurent Bossavit <laurent.bossavit@...> wrote:
                              > > This is a typical inconsistent state bug. Inspection is the
                              > > cheapest tool to find these.
                              >
                              > Rings true. So we might conclude the following.
                              >
                              > Some classes of problems are "hard to test" in the sense that test-
                              > first will not completely ensure reliability.

                              The challenge with this sort of problem isn't so much that is "hard to
                              test" as that "if you think of the test, it's because you don't have
                              the bug". These are bugs that occur in the blind spot.

                              Regards,

                              Rob


                              __________________________________________________
                              Do You Yahoo!?
                              Get email at your own domain with Yahoo! Mail.
                              http://personal.mail.yahoo.com/
                            Your message has been successfully submitted and would be delivered to recipients shortly.