Ticket #6447 (closed: fixed)

Opened 8 years ago

Last modified 5 years ago

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:1 Changed 7 years ago by Alex Buts

  • Milestone changed from Release 2.5 to Release 2.6

comment:2 Changed 7 years ago by Alex Buts

  • Blocked By 7200 added

comment:3 Changed 7 years ago by Nick Draper

  • Component changed from VATES to User Interface

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:15 Changed 7 years ago by Alex Buts

  • Status changed from reopened to inprogress

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:18 Changed 7 years ago by Karl Palmen

  • Status changed from verify to verifying

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:29 Changed 7 years ago by Martyn Gigg

  • Status changed from verify to verifying

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.

Last edited 7 years ago by Karl Palmen (previous) (diff)

comment:32 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 7293

Note: See TracTickets for help on using tickets.