Page MenuHomePhabricator

Refactor LevelWindowManagerTest
Open, NormalPublic

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

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.