Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[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.

(cherry picked from commit 2f58ceb)
  • Loading branch information
jglick committed Jul 11, 2014
1 parent 525793d commit c3e7329
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 13 deletions.
Expand Up @@ -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;

Expand Down Expand Up @@ -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());
Expand Down
1 change: 1 addition & 0 deletions core/src/test/java/jenkins/model/lazy/FakeMap.java
Expand Up @@ -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();

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Jul 11, 2014

Member

Left over debug statement?

This comment has been minimized.

Copy link
@jglick

jglick Aug 29, 2014

Author Member

Yeah, but useful to uncomment when actually making any changes.

return new Build(Integer.parseInt(n),id);
}
}
Expand Down

0 comments on commit c3e7329

Please sign in to comment.