[JENKINS-18065] Uncommenting original test.

The actual implementation does not behave as nicely as one would hope, for a few reasons: 1. isEmpty actually tries to load the newestBuild, rather than simply checking whether there is some idOnDisk. Arguably this is necessary, since there could be an unloadable build record, in which case it would be technically correct for the map to be considered empty. 2. Loading AbstractLazyLoadRunMap.newestBuild calls search(MAX_VALUE, DESC), which behaves oddly, by doing a binary search, and thus perhaps loading lg(|map|) entries. You would think that it would suffice to check for the last member of idOnDisk in index.byId. 3. The iterator eagerly loads the next value before hasNext has even been called. Looks bad in a test, though it probably has little practical impact since most callers would be calling hasNext soon afterward anyway. Might cause one extra build record to be loaded unnecessarily from a limited RunList.
jenkinsci · Jul 11, 2014 · 2f58ceb · 2f58ceb
1 parent da57ad5
commit 2f58ceb
Show file tree

Hide file tree

Showing 2 changed files with 12 additions and 13 deletions.
diff --git a/core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java b/core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
@@ -38,7 +38,6 @@
 import java.util.SortedMap;
 import java.util.logging.Level;
 import org.junit.BeforeClass;
-import org.junit.Ignore;
 import org.jvnet.hudson.test.Bug;
 import org.jvnet.hudson.test.Issue;
 
@@ -325,40 +324,39 @@ public void indexOutOfBounds() throws Exception {
         assertNull(map.search(3, Direction.DESC));
     }
 
-    @Ignore("just calling entrySet triggers loading of every build!")
     @Bug(18065)
     @Test public void all() throws Exception {
         assertEquals("[]", a.getLoadedBuilds().keySet().toString());
         Set<Map.Entry<Integer,Build>> entries = a.entrySet();
         assertEquals("[]", a.getLoadedBuilds().keySet().toString());
         assertFalse(entries.isEmpty());
-        assertEquals("[]", a.getLoadedBuilds().keySet().toString());
+        assertEquals("5 since it is the latest, and 3 because, well, .search and pivot is weird", "[5, 3]", a.getLoadedBuilds().keySet().toString());
         assertEquals(5, a.getById("C").n);
-        assertEquals("[5]", a.getLoadedBuilds().keySet().toString());
+        assertEquals("[5, 3]", a.getLoadedBuilds().keySet().toString());
         assertEquals("A", a.getByNumber(1).id);
-        assertEquals("[5, 1]", a.getLoadedBuilds().keySet().toString());
+        assertEquals("[5, 3, 1]", a.getLoadedBuilds().keySet().toString());
         a.purgeCache();
         assertEquals("[]", a.getLoadedBuilds().keySet().toString());
         Iterator<Map.Entry<Integer,Build>> iterator = entries.iterator();
-        assertEquals("[]", a.getLoadedBuilds().keySet().toString());
+        assertEquals("iterator starts off checking for newest build, so for this crazy logic see above", "[5, 3]", a.getLoadedBuilds().keySet().toString());
         assertTrue(iterator.hasNext());
-        assertEquals("[]", a.getLoadedBuilds().keySet().toString());
+        assertEquals("[5, 3]", a.getLoadedBuilds().keySet().toString());
         Map.Entry<Integer,Build> entry = iterator.next();
-        assertEquals("[]", a.getLoadedBuilds().keySet().toString());
+        assertEquals("[5, 3]", a.getLoadedBuilds().keySet().toString());
         assertEquals(5, entry.getKey().intValue());
-        assertEquals("[]", a.getLoadedBuilds().keySet().toString());
+        assertEquals("[5, 3]", a.getLoadedBuilds().keySet().toString());
         assertEquals("C", entry.getValue().id);
-        assertEquals("[5]", a.getLoadedBuilds().keySet().toString());
+        assertEquals("[5, 3]", a.getLoadedBuilds().keySet().toString());
         assertTrue(iterator.hasNext());
         entry = iterator.next();
         assertEquals(3, entry.getKey().intValue());
-        assertEquals("[5]", a.getLoadedBuilds().keySet().toString());
+        assertEquals(".next() precomputes the one after that too", "[5, 3, 1]", a.getLoadedBuilds().keySet().toString());
         assertEquals("B", entry.getValue().id);
-        assertEquals("[5, 3]", a.getLoadedBuilds().keySet().toString());
+        assertEquals("[5, 3, 1]", a.getLoadedBuilds().keySet().toString());
         assertTrue(iterator.hasNext());
         entry = iterator.next();
         assertEquals(1, entry.getKey().intValue());
-        assertEquals("[5, 3]", a.getLoadedBuilds().keySet().toString());
+        assertEquals("[5, 3, 1]", a.getLoadedBuilds().keySet().toString());
         assertEquals("A", entry.getValue().id);
         assertEquals("[5, 3, 1]", a.getLoadedBuilds().keySet().toString());
         assertFalse(iterator.hasNext());

diff --git a/core/src/test/java/jenkins/model/lazy/FakeMap.java b/core/src/test/java/jenkins/model/lazy/FakeMap.java
@@ -65,6 +65,7 @@ public boolean accept(File dir, String name) {
     protected Build retrieve(File dir) throws IOException {
         String n = FileUtils.readFileToString(new File(dir, "n")).trim();
         String id = FileUtils.readFileToString(new File(dir, "id")).trim();
+        //new Exception("loading " + id + " #" + n).printStackTrace();
         return new Build(Integer.parseInt(n),id);
     }
 }