Make separate unit tests for MDBoxFlatTree class

[Alex Buts]

MDBox flat tree class is the extract of the common code, taken from SaveMD LoadMD and MergeMD files algorithms. As the ectract and common code it is tested by appropriate algorithm tests but it is much better and convenient to test this class through its specific tester.

[Nick Draper]

Moved to backlog at the code freeze for R2.6

[Alex Buts]

refs #6447 Existing test fixed and enabled

Changeset: 7fe03bc1a9908d752ed62065fc680d2ffa4be233

[Alex Buts]

refs #6447 most tests and some controversial changes

to the MDBoxFlatTree methors (boxID !=boxNum?

Changeset: 87f72a323751a2cf76f31c9e53ee785301270ac9

[Alex Buts]

refs #6447 fixing Unix warning

Changeset: 8f97c6f609288448ceb9f1ac5564e010100907e5

[Alex Buts]

refs #6447 fixed erroneous test suite name

Changeset: 3e3dfd7d3e1f4ef484dff480ab1dee7d73719060

[Alex Buts]

refs #6447 Thorough test for all suspicious places.

Tested the pieces of code where changes can be expected

Changeset: 65611fdd0cf7259582247daa798dd65e5bdff3d1

[Alex Buts]

refs #6447 Doxygen error

Changeset: ab9ac42f6795d06a1cb370a8e6f7e5c8d5d1ebc7

[Alex Buts]

Nothing to test here -- new unit test added and it probably runs fine.

Just merge this into the master.

[Karl Palmen]

Code looks good, but BoxController::fromXMLString needs the pDoc->Release() when pDoc is no longer needed, else there might be memory leakage.

I'd put it into master after pDoc->release() has been added.

[Alex Buts]

refs #6447 Added pDoc->release()

Hate to use features I do not understand to the code I have not written but it seems does not do any harm

Changeset: 45deb7acae9a3b9b149f01f7f6da6a61910f6a53

[Karl Palmen]

I find it also needed doing in BoxController::toXMLString().

[Alex Buts]

refs #6447 removed unnecessary memory release operators from BC

Changeset: c13d066337d040f5a1fb76e82b7c99aa5a75f69e

[Alex Buts]

According to the samples of memory management provided for POCO xml (​http://pocoproject.org/slides/170-XML.pdf), the changes to the code suggested by the tester are unnecessary.

I am not an expert in xml, so another ticket (#8053) was created to check if my fast opinion is indeed correct.

[Martyn Gigg]

Alex is correct here. The pDoc->release call is not necessary because the code uses a Poco::AutoPtr which takes care of the memory management. Other places in the code, like the InstrumentDefinitionParser, use bare Poco::Document pointers and these do require the release calls. This means #8053 is now invalid.

It's good that the test coverage is improving in these areas. I was going to pass the ticket but I noticed a few things about the code that I think need a quick tidying up.

First, the structure of the class in MDBoxFlatTreeTest.h is not consistent with our standard class structure. The structure should be:

class MDBoxFlatTreeTest
{
public:
  // This pair of boilerplate methods prevent the suite being created statically
  // This means the constructor isn't called when running other tests
  static MDBoxFlatTreeTest  *createSuite() { return new MDBoxFlatTreeTest (); }
  static void destroySuite( MDBoxFlatTreeTest  *suite ) { delete suite; }

  MDBoxFlatTreeTest()
  {
    //constructor code
  }
  
  //
  // -- test methods -- 
  //

private:
  // -- helper methods --

  // -- member variables --
};

Second, BoxController::operator== has some commented code that needs removing.

[Alex Buts]

refs #6447 Changed the order of operations in the test suite suggested

by tester. Though I believe the previous order was more suitable highlighting the internal variables used by the suite right from the beginning.

Changeset: a1dda14386d14a394760d05658cb6c655fafd526

[Alex Buts]

refs #6447 Made better comments around (from) commented piece of code

in BoxController.cpp == operator. Comments are necessary to highlight the assumptions, made when defining the operator.

Changeset: 14e41694bbced053a0a1c44aa35e293ecec496d1

[Alex Buts]

refs #6447 Syntax improvement.

Changeset: 0a8cd7d8330ab9c48acad926c1571c216ac20efa

[Martyn Gigg]

Thanks for making the improvements. As with other classes, and particularly in tests, the highlights should be the methods/tests that the class is performing not the internal stuff used to make it happen.

[Martyn Gigg]

Merge remote-tracking branch 'origin/feature/6447_UnitTest4MDBoxTree'

Full changeset: ​7fc56fdc37e36b9e1e93aab71b715371ca89ee23

[Karl Palmen]

Ticket #8053 has been extended to cover cases mentioned in comment:23 and made into a maintenance ticket.

[Stuart Campbell]

This ticket has been transferred to github issue ​7293