Ticket #6447 (closed: fixed)
Make separate unit tests for MDBoxFlatTree class
Reported by: | Alex Buts | Owned by: | Alex Buts |
---|---|---|---|
Priority: | major | Milestone: | Release 3.0 |
Component: | Framework | Keywords: | |
Cc: | Blocked By: | #7200 | |
Blocking: | Tester: | Martyn Gigg |
Description
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.
Change History
comment:4 Changed 7 years ago by Nick Draper
- Milestone changed from Release 2.6 to Backlog
Moved to backlog at the code freeze for R2.6
comment:5 Changed 7 years ago by Alex Buts
- Status changed from new to inprogress
- Component changed from User Interface to Framework
- Milestone changed from Backlog to Release 3.0
comment:6 Changed 7 years ago by Alex Buts
refs #6447 Existing test fixed and enabled
Changeset: 7fe03bc1a9908d752ed62065fc680d2ffa4be233
comment:7 Changed 7 years ago by Alex Buts
refs #6447 most tests and some controversial changes
to the MDBoxFlatTree methors (boxID !=boxNum?
Changeset: 87f72a323751a2cf76f31c9e53ee785301270ac9
comment:8 Changed 7 years ago by Alex Buts
refs #6447 fixing Unix warning
Changeset: 8f97c6f609288448ceb9f1ac5564e010100907e5
comment:9 Changed 7 years ago by Alex Buts
refs #6447 fixed erroneous test suite name
Changeset: 3e3dfd7d3e1f4ef484dff480ab1dee7d73719060
comment:10 Changed 7 years ago by Alex Buts
refs #6447 Thorough test for all suspicious places.
Tested the pieces of code where changes can be expected
Changeset: 65611fdd0cf7259582247daa798dd65e5bdff3d1
comment:11 Changed 7 years ago by Alex Buts
refs #6447 Doxygen error
Changeset: ab9ac42f6795d06a1cb370a8e6f7e5c8d5d1ebc7
comment:12 Changed 7 years ago by Alex Buts
- Status changed from inprogress to verify
- Resolution set to fixed
Nothing to test here -- new unit test added and it probably runs fine.
Just merge this into the master.
comment:13 Changed 7 years ago by Karl Palmen
- Status changed from verify to verifying
- Tester set to Karl Palmen
comment:14 Changed 7 years ago by Karl Palmen
- Status changed from verifying to reopened
- Resolution fixed deleted
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.
comment:16 Changed 7 years ago by 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
comment:17 Changed 7 years ago by Alex Buts
- Status changed from inprogress to verify
- Resolution set to fixed
comment:19 Changed 7 years ago by Karl Palmen
- Status changed from verifying to reopened
- Resolution fixed deleted
I find it also needed doing in BoxController::toXMLString().
comment:20 Changed 7 years ago by Alex Buts
- Status changed from reopened to inprogress
refs #6447 removed unnecessary memory release operators from BC
Changeset: c13d066337d040f5a1fb76e82b7c99aa5a75f69e
comment:21 Changed 7 years ago by Alex Buts
- Status changed from inprogress to verify
- Resolution set to fixed
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.
comment:22 Changed 7 years ago by Martyn Gigg
- Status changed from verify to verifying
- Tester changed from Karl Palmen to Martyn Gigg
comment:23 Changed 7 years ago by Martyn Gigg
- Status changed from verifying to reopened
- Resolution fixed deleted
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.
comment:24 Changed 7 years ago by Alex Buts
- Status changed from reopened to inprogress
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
comment:25 Changed 7 years ago by 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
comment:26 Changed 7 years ago by Alex Buts
refs #6447 Syntax improvement.
Changeset: 0a8cd7d8330ab9c48acad926c1571c216ac20efa
comment:27 Changed 7 years ago by Alex Buts
- Status changed from inprogress to verify
- Resolution set to fixed
comment:28 Changed 7 years ago by 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.
comment:30 Changed 7 years ago by Martyn Gigg
- Status changed from verifying to closed
Merge remote-tracking branch 'origin/feature/6447_UnitTest4MDBoxTree'
Full changeset: 7fc56fdc37e36b9e1e93aab71b715371ca89ee23
comment:31 Changed 7 years ago by Karl Palmen
Ticket #8053 has been extended to cover cases mentioned in comment:23 and made into a maintenance ticket.
comment:32 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 7293