Page MenuHomePhabricator

Refactor LevelWindowManagerTest
Closed, ResolvedPublic

Description

The mitkLevelWindowManagerTest should be refactored because of several reasons:

  1. It uses the old MITK test style instead of the CppUnit test framework
  2. It does not test all property changes (e.g. no layer change, selected change, image rendering mode change ...)
  3. It uses random tests (random value changes) which is something that shouldn't be done.

Random tests (monkey testing) are problematic in my opinion: Failing tests are hard to debug so it might take a long time to actually find the error. Random values are hard to reproduce, which renders them useless in this scenario (this is even more so true since no seed is provided, leading to "real" random values).
They might have their power when it comes to certain types of tests but in this scenario I think a clean definition of what is tested is more helpful.
Additionally the code is very hard to understand since documentation is missing. Having clear and cleanly defined test cases can also be used for self-documentation.

What's your opinion on this?

Revisions and Commits

rMITK MITK
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision

Event Timeline

kalali triaged this task as High priority.Jan 26 2021, 9:31 PM
kalali created this task.

I opened a review for my modifications / suggestions for a LevelWindowManagerTest. Please have a look at it: D461
I have explicitly two problems with the current test:

  1. I need to load the three images / datasets for each tests since it is not possible to keep those images in memory for every new tests. Does anyone know how to solve this problem?
  2. I randomly get a failed test in line 287 / 288
m_DataNode2->SetSelected(true);
CPPUNIT_ASSERT_MESSAGE("\"imageForLevelWindow\" property not correctly set", AssertImageForLevelWindowProperty(false, true, false));

My first assumption for problem 2 is that there is some non-deterministic decision being made while retrieving the nodes (subset) of the data storage. Could that be? I can go into more detail what is happening after m_DataNode2->SetSelected(true); but this is my first idea (nodes are being retrieved to check their properties and if a specific property is found, that node is used - so it depends on the order of the retrieved nodes).

Also: The tests revealed some errors in the LevelWindowManager which will be addressed in an own task.

kislinsk added a revision: Restricted Differential Revision.Jan 29 2021, 11:23 AM
kalali lowered the priority of this task from High to Normal.Feb 12 2021, 12:11 PM
kalali edited projects, added MITK; removed MITK (v2021.02).
kalali moved this task from Backlog to Cycle on the MITK (v2021.10) board.

Since I modified and merged T28250 I wanted to continue here. I still get failed tests in line 288 and in line 320:

assertion failed
- Expression: AssertImageForLevelWindowProperty(false, true, false)
- "imageForLevelWindow" property not correctly set
assertion failed
- Expression: AssertImageForLevelWindowProperty(true, false, false)
- "imageForLevelWindow" property not correctly set

@kislinsk Any ideas on this?

My first assumption for problem 2 is that there is some non-deterministic decision being made while retrieving the nodes (subset) of the data storage. Could that be? I can go into more detail what is happening after m_DataNode2->SetSelected(true); but this is my first idea (nodes are being retrieved to check their properties and if a specific property is found, that node is used - so it depends on the order of the retrieved nodes).

Hm, I cannot answer this without looking into this in detail but a first step could be to improve the AssertImageForLevelWindowProperty function or maybe even encapsule the CPPUNIT_ASSERT_MESSAGE calls in that function instead the other way around since the error message one does receive currently doesn't tell much about what actually failed since it could be any or all of the three checks.

Yes, I agree that this is not the most straight forward way to do it. There was a reasoning behind it to do it that way.
I will think about it - but currently it seems as if something is not correct with the tests, in a way that I am actually expecting wrong test results - so I need to rethink the expected results and change the true / false arguments accordingly.

Ok so I found a bug for the test in line 320. This showed some complex behavior with the level window manager, which I don't want to address here.
However, the test in line 288 still randomly fails - when debugging in that line the test does not fail but fails in line 292.

So I modified the "Assert"-functions to directly encapsule the CPPUNIT_ASSERT_MESSAGE calls in that function as you suggested. The error still occurs and now the problem is that I can't see where the error occurs because it happens inside the "Assert"-function (which is the same for each test). So that is not an option. Any other suggestions? I could split the test to test each node individually.

We also found out that the current LevelWindowManagerTest fails: https://cdash.mitk.org/testDetails.php?test=774190&build=12215
The problem is the modified LevelWindowManager in combination with the mentioned random tests: The random tests can lead to all nodes having an invalid rendering mode. The LevelWindowManager has been modified in T28250 which conflicts with the current level window manager test.

So the old test assumes something which is not valid with the new LevelWindowManager; however, I think the assumption was wrong in the beginning, because the LevelWindowmanager was buggy.
I will make sure that such a test is included in the new LevelWindowManagerTest.

@kislinsk:
So I took a closer look into this and I found out the following:
Inside mitkLevelWindowTest:

  • m_DataNode1: 0x00000221c86a80f0
  • m_DataNode2: 0x00000221c86a6930
  • m_DataNode3: 0x00000221c86a7bf0

Inside mitkLevelWindowManager (using mitk::DataStorage::GetSubset):

  • node[0]: 0x00000221c86a6930 (is actually m_DataNode2)
  • node[1]: 0x00000221c86a7bf0 (is actually m_DataNode3)
  • node[2]: 0x00000221c86a80f0 (is actually m_DataNode1)

The function mitk::LevelWindowManager::SetSelectedImages goes through this set of nodes and uses the last one found (selected, visible, valid rendering mode) for setting the levelwindow property.
So in this case the levelwindow property is set for m_DataNode1 because both nodes m_DataNode1 and m_DataNode2 were set to visible and selected but m_DataNode1 was the last one found. However, the test expects m_DataNode2 because that's the node that was last selected.

Running a second time results in the following:
Inside mitkLevelWindowTest:

  • m_DataNode1: 0x0000027d0b07b650
  • m_DataNode2: 0x0000027d0b07c690
  • m_DataNode3: 0x0000027d0b07bb50

Inside mitkLevelWindowManager (using mitk::DataStorage::GetSubset):

  • node[0]: 0x0000027d0b07b650 (is actually m_DataNode1)
  • node[1]: 0x0000027d0b07bb50 (is actually m_DataNode3)
  • node[2]: 0x0000027d0b07c690 (is actually m_DataNode2)

It can be seen that the retrieved set is ordered differently which will result in a different node being the last one found.
So in this case the levelwindow property is set for m_DataNode2 because both nodes m_DataNode1 and m_DataNode2 were set to visible and selected but m_DataNode2 was the last one found. The test expects m_DataNode2 because that's the node that was last selected - so the test succeeds.

I wonder why the returned subset using mitk::DataStorage::GetSubset works that way and why this is not a problem anywhere else in the code.

I used the following code snippet to test the retrieval of the data nodes

void setUp() override
{
  for (int i = 0; i < 3; ++i)
  {
    m_LevelWindowManager = mitk::LevelWindowManager::New();
    m_DataManager = mitk::StandaloneDataStorage::New();

    CPPUNIT_ASSERT_NO_THROW_MESSAGE("DataStorage could not be set for the new level window manager", m_LevelWindowManager->SetDataStorage(m_DataManager));
    CPPUNIT_ASSERT_MESSAGE("DataStorage could not be retrieved from the new level window manager", m_DataManager == m_LevelWindowManager->GetDataStorage());

    m_ImagePath1 = GetTestDataFilePath("Pic3D.nrrd");
    m_ImagePath2 = GetTestDataFilePath("UltrasoundImages/4D_TEE_Data_MV.dcm");
    m_ImagePath3 = GetTestDataFilePath("RenderingTestData/defaultWatermark.png");

    m_DataNode1 = mitk::IOUtil::Load(m_ImagePath1, *m_DataManager)->GetElement(0);
    m_DataNode2 = mitk::IOUtil::Load(m_ImagePath2, *m_DataManager)->GetElement(0);
    m_DataNode3 = mitk::IOUtil::Load(m_ImagePath3, *m_DataManager)->GetElement(0);

    mitk::DataStorage::SetOfObjects::ConstPointer allNodes = m_DataManager->GetAll();
    for (mitk::DataStorage::SetOfObjects::ConstIterator it = allNodes->Begin(); it != allNodes->End(); ++it)
    {
      mitk::DataNode::Pointer node = it->Value();
      std::cout << node->GetName() << "\n";
    }
  }
}

The result is the following:

defaultWatermark
Pic3D
4D_TEE_Data_MV.dcm
Pic3D
4D_TEE_Data_MV.dcm
defaultWatermark
4D_TEE_Data_MV.dcm
Pic3D
defaultWatermark

Just as a side note. As far as I know, the data storage interace gives no guarantees regarding the order (aplied sorting criteria) of returned node sets. So any implementation that relies on such an order is likely to fail.

The ony guarantee that you have is that predicates are regarded if you search for subsets or Parent-Child-relations if you provide a parent node,

That's good to hear so I'm not missing something. However, I never paid attention to this and I'm still wondering why that's the case.
So in my tests I need to find a way to predict the correct outcome in order to make the test succeed.

The refactored tests have been merged into dev; the CDash tests are also green again!