Vojtech Szocs has posted comments on this change. Change subject: webadmin,userportal: Fix client-side sort (ArrayList approach) ......................................................................
Patch Set 2: (3 comments) http://gerrit.ovirt.org/#/c/28392/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModel.java: Line 84: if (items == null || comparator == null) { Line 85: return items; Line 86: } Line 87: Line 88: List<T> sortedList = new ArrayList<T>() { > Since we decided to go with the simplest implementation possible, pleas rem Agreed - I'll remove these overrides. Line 89: Line 90: private static final long serialVersionUID = 499355412389856325L; Line 91: Line 92: @Override http://gerrit.ovirt.org/#/c/28392/2/frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModelTest.java File frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModelTest.java: Line 83: } Line 84: Line 85: @Before Line 86: public void setUp() { Line 87: this.model = new SortedListModel<TestItem>() { > If we keep this test class, consider using Mockito.spy() and mocking these In general, you are right. In this particular case, spying won't help much because lookupConfigurator() + lookupLogger() methods are called right from within Model constructor. In other words, following will not work: SortedListModel<TestItem> realModel = new SortedListModel<TestItem>(); SortedListModel<TestItem> testedModel = Mockito.spy(realModel); doReturn(configuratorMock).when(testedModel).lookupConfigurator(); doReturn(loggerMock).when(testedModel).lookupLogger(); because NPE occurs at first line. In other words, spying on objects (in order to stub some methods) works only if you're able to create the instance to spy upon on your own, which is not the case here - NPE due to hidden static method calls from within lookupConfigurator() + lookupLogger() methods. For reasons mentioned above, I'll keep the original code here. Line 88: @Override Line 89: protected Configurator lookupConfigurator() { Line 90: return null; Line 91: } Line 115: assertNull(sorted); Line 116: } Line 117: Line 118: @Test Line 119: public void testSortItems_retainOrder() { > This test is not required anymore, and I'm not sure there's much else to te I think that testSortItems_retainOrder is still relevant - the part which tests if items are sorted after being set into the list model. Agreed on adding separate test for switching comparators. Line 120: List<TestItem> initial = Arrays.asList(ITEM_2_1, ITEM_2_2, ITEM_1_2, ITEM_1_1); Line 121: List<TestItem> expected = Arrays.asList(ITEM_1_2, ITEM_1_1, ITEM_2_1, ITEM_2_2); Line 122: Collection<TestItem> sorted = model.sortItems(initial); Line 123: -- To view, visit http://gerrit.ovirt.org/28392 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I437e9dd7ff2afdb2d87fb0ba6c17dcb081cd965d Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vojtech Szocs <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Einav Cohen <[email protected]> Gerrit-Reviewer: Frank Kobzik <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
