Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix py_SubjectHierarchyGenericSelfTest test #7288

Closed
jcfr opened this issue Oct 18, 2023 · 4 comments · Fixed by #7289
Closed

Fix py_SubjectHierarchyGenericSelfTest test #7288

jcfr opened this issue Oct 18, 2023 · 4 comments · Fixed by #7289
Assignees
Labels
Type: Bug Something isn't working correctly

Comments

@jcfr
Copy link
Member

jcfr commented Oct 18, 2023

Summary

The test py_SubjectHierarchyGenericSelfTest passed on all platform when executed against a build of Slicer 31ba894 and started to fail with Slicer 80ad0a0.

CDash link https://slicer.cdash.org/test/25604578 https://slicer.cdash.org/test/25610246
Slicer Source 31ba894 80ad0a0

Useful links

Steps to reproduce

ctest -C Release -R py_SubjectHierarchyGenericSelfTest

[...]

308: FAIL: test_SubjectHierarchyGenericSelfTest_FullTest1 (SubjectHierarchyGenericSelfTest.SubjectHierarchyGenericSelfTestTest)
308: ----------------------------------------------------------------------
308: Traceback (most recent call last):
308:   File "/home/jcfr/Projects/Slicer-Release/Slicer-build/lib/Slicer-5.5/qt-scripted-modules/SubjectHierarchyGenericSelfTest.py", line 113, in test_SubjectHierarchyGenericSelfTest_FullTest1
308:     self.section_AttributeFilters()
308:   File "/home/jcfr/Projects/Slicer-Release/Slicer-build/lib/Slicer-5.5/qt-scripted-modules/SubjectHierarchyGenericSelfTest.py", line 565, in section_AttributeFilters
308:     testAttributeFilters(shProxyModel, shProxyModel)
308:   File "/home/jcfr/Projects/Slicer-Release/Slicer-build/lib/Slicer-5.5/qt-scripted-modules/SubjectHierarchyGenericSelfTest.py", line 480, in testAttributeFilters
308:     self.assertEqual(shProxyModel.acceptedItemCount(shNode.GetSceneItemID()), 5)
308: AssertionError: 6 != 5

Expected behavior

Test should pass on all platform

@jcfr jcfr added the Type: Bug Something isn't working correctly label Oct 18, 2023
@cpinter
Copy link
Member

cpinter commented Oct 18, 2023

It seems that the failure is due to the fact that the showEmptyHierarchyItems flag is on in the proxy model. Trying to see why it is enabled.

@cpinter
Copy link
Member

cpinter commented Oct 18, 2023

It seems that it has been always on by default, and it seems correct that the empty folder shows up, so the item count of 6 that made the test fail is the correct number. I propose to fix the test by adjusting the expected value.

The six items:
image
The four markup nodes have the node attribute that was filtered. Their parent shows up anyway, and the empty folder shows up due to the (by default) enabled showEmptyHierarchyItems flag.

@jcfr
Copy link
Member Author

jcfr commented Oct 18, 2023

Updating the expected value makes sense and would be consistent with the new default behavior.

Before #7214 After #7214
image image

I also suggest to update to check that setting showEmptyHierarchyItems works as expected.

After loading SubjectHierarchyAttributeFilterTestScene.mrb, I used the following convenient snippet based of the current test:

shTreeView = slicer.qMRMLSubjectHierarchyTreeView()
shTreeView.setMRMLScene(slicer.mrmlScene)
shTreeView.show()
shProxyModel = shTreeView.sortFilterProxyModel()
shNode = slicer.mrmlScene.GetSubjectHierarchyNode()

assert shProxyModel.acceptedItemCount(shNode.GetSceneItemID()) == 9

shProxyModel.includeNodeAttributeNamesFilter = ['Markups.MovingInSliceView']

# By default, shProxyModel.showEmptyHierarchyItems is True
assert shProxyModel.acceptedItemCount(shNode.GetSceneItemID()) == 6

shProxyModel.showEmptyHierarchyItems = False
assert shProxyModel.acceptedItemCount(shNode.GetSceneItemID()) == 5

cpinter added a commit to cpinter/Slicer that referenced this issue Oct 18, 2023
- The test assumed incorrect shown item count including the empty folder (which flag is enabled by default), expected item counts has been fixed
- Add test for the showEmptyHierarchyItems flag, which has not been tested until now

Re Slicer#7288
@cpinter
Copy link
Member

cpinter commented Oct 18, 2023

PR ready to review.

Yes I also realized the showEmptyHierarchyItems flag was not tested so I added a small test at the end of the attributes test section.

jcfr added a commit to cpinter/Slicer that referenced this issue Oct 18, 2023
Follow-up of 5f7ae8d (ENH: Improve empty folder visibility logic):

- The test assumed incorrect shown item count including the empty folder (which flag is enabled by default), expected item counts has been fixed
- Update testAttributeFilters to check for showEmptyHierarchyItems flag, which has not been tested until now

Re Slicer#7288

Co-authored-by: Jean-Christophe Fillion-Robin <jchris.fillionr@kitware.com>
@jcfr jcfr linked a pull request Oct 18, 2023 that will close this issue
jcfr added a commit to cpinter/Slicer that referenced this issue Oct 18, 2023
Follow-up of 5f7ae8d (ENH: Improve empty folder visibility logic):

- Update test to consider that empty folders are now visible by default.
- Update testAttributeFilters to check for showEmptyHierarchyItems set
  to False and True.

Fixes Slicer#7288

Co-authored-by: Jean-Christophe Fillion-Robin <jchris.fillionr@kitware.com>
jcfr added a commit that referenced this issue Oct 18, 2023
Follow-up of 5f7ae8d (ENH: Improve empty folder visibility logic):

- Update test to consider that empty folders are now visible by default.
- Update testAttributeFilters to check for showEmptyHierarchyItems set
  to False and True.

Fixes #7288

Co-authored-by: Jean-Christophe Fillion-Robin <jchris.fillionr@kitware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working correctly
Development

Successfully merging a pull request may close this issue.

2 participants