update lru k tests for new evict signature

test/buffer/lru_k_replacer_test.cpp

@@ -16,10 +16,16 @@
 
 namespace bustub {
 
-TEST(LRUKReplacerTest, DISABLED_SampleTest) {
+TEST(LRUKReplacerTest, SampleTest) {
+  // Note that comparison with `std::nullopt` always results in `false`, and if the optional type actually does contain
+  // a value, the comparison will compare the inner value.
+  // See: https://devblogs.microsoft.com/oldnewthing/20211004-00/?p=105754
+  std::optional<frame_id_t> frame;
+
+  // Initialize the replacer.
   LRUKReplacer lru_replacer(7, 2);
 
-  // Scenario: add six elements to the replacer. We have [1,2,3,4,5]. Frame 6 is non-evictable.
+  // Add six frames to the replacer. We now have frames [1, 2, 3, 4, 5]. We set frame 6 as non-evictable.
   lru_replacer.RecordAccess(1);
   lru_replacer.RecordAccess(2);
   lru_replacer.RecordAccess(3);
@@ -32,25 +38,24 @@ TEST(LRUKReplacerTest, DISABLED_SampleTest) {
   lru_replacer.SetEvictable(4, true);
   lru_replacer.SetEvictable(5, true);
   lru_replacer.SetEvictable(6, false);
+
+  // The size of the replacer is the number of frames that can be evicted, _not_ the total number of frames entered.
   ASSERT_EQ(5, lru_replacer.Size());
 
-  // Scenario: Insert access history for frame 1. Now frame 1 has two access histories.
-  // All other frames have max backward k-dist. The order of eviction is [2,3,4,5,1].
+  // Record an access for frame 1. Now frame 1 has two accesses total.
   lru_replacer.RecordAccess(1);
+  // All other frames now share the maximum backward k-distance. Since we use timestamps to break ties, where the first
+  // to be evicted is the frame with the oldest timestamp, the order of eviction should be [2, 3, 4, 5, 1].
 
-  // Scenario: Evict three pages from the replacer. Elements with max k-distance should be popped
-  // first based on LRU.
-  int value;
-  lru_replacer.Evict(&value);
-  ASSERT_EQ(2, value);
-  lru_replacer.Evict(&value);
-  ASSERT_EQ(3, value);
-  lru_replacer.Evict(&value);
-  ASSERT_EQ(4, value);
+  // Evict three pages from the replacer.
+  // To break ties, we use LRU with respect to the oldest timestamp, or the least recently used frame.
+  ASSERT_EQ(2, lru_replacer.Evict());
+  ASSERT_EQ(3, lru_replacer.Evict());
+  ASSERT_EQ(4, lru_replacer.Evict());
   ASSERT_EQ(2, lru_replacer.Size());
+  // Now the replacer has the frames [5, 1].
 
-  // Scenario: Now replacer has frames [5,1].
-  // Insert new frames 3, 4, and update access history for 5. We should end with [3,1,5,4]
+  // Insert new frames [3, 4], and update the access history for 5. Now, the ordering is [3, 1, 5, 4].
   lru_replacer.RecordAccess(3);
   lru_replacer.RecordAccess(4);
   lru_replacer.RecordAccess(5);
@@ -59,40 +64,59 @@ TEST(LRUKReplacerTest, DISABLED_SampleTest) {
   lru_replacer.SetEvictable(4, true);
   ASSERT_EQ(4, lru_replacer.Size());
 
-  // Scenario: continue looking for victims. We expect 3 to be evicted next.
-  lru_replacer.Evict(&value);
-  ASSERT_EQ(3, value);
+  // Look for a frame to evict. We expect frame 3 to be evicted next.
+  ASSERT_EQ(3, lru_replacer.Evict());
   ASSERT_EQ(3, lru_replacer.Size());
 
-  // Set 6 to be evictable. 6 Should be evicted next since it has max backward k-dist.
+  // Set 6 to be evictable. 6 Should be evicted next since it has the maximum backward k-distance.
   lru_replacer.SetEvictable(6, true);
   ASSERT_EQ(4, lru_replacer.Size());
-  lru_replacer.Evict(&value);
-  ASSERT_EQ(6, value);
+  ASSERT_EQ(6, lru_replacer.Evict());
   ASSERT_EQ(3, lru_replacer.Size());
 
-  // Now we have [1,5,4]. Continue looking for victims.
+  // Mark frame 1 as non-evictable. We now have [5, 4].
   lru_replacer.SetEvictable(1, false);
+
+  // We expect frame 5 to be evicted next.
   ASSERT_EQ(2, lru_replacer.Size());
-  ASSERT_EQ(true, lru_replacer.Evict(&value));
-  ASSERT_EQ(5, value);
+  ASSERT_EQ(5, lru_replacer.Evict());
   ASSERT_EQ(1, lru_replacer.Size());
 
-  // Update access history for 1. Now we have [4,1]. Next victim is 4.
+  // Update the access history for frame 1 and make it evictable. Now we have [4, 1].
   lru_replacer.RecordAccess(1);
   lru_replacer.RecordAccess(1);
   lru_replacer.SetEvictable(1, true);
   ASSERT_EQ(2, lru_replacer.Size());
-  ASSERT_EQ(true, lru_replacer.Evict(&value));
-  ASSERT_EQ(value, 4);
 
+  // Evict the last two frames.
+  ASSERT_EQ(4, lru_replacer.Evict());
+  ASSERT_EQ(1, lru_replacer.Size());
+  ASSERT_EQ(1, lru_replacer.Evict());
+  ASSERT_EQ(0, lru_replacer.Size());
+
+  // Insert frame 1 again and mark it as non-evictable.
+  lru_replacer.RecordAccess(1);
+  lru_replacer.SetEvictable(1, false);
+  ASSERT_EQ(0, lru_replacer.Size());
+
+  // A failed eviction should not change the size of the replacer.
+  frame = lru_replacer.Evict();
+  ASSERT_EQ(false, frame.has_value());
+
+  // Mark frame 1 as evictable again and evict it.
+  lru_replacer.SetEvictable(1, true);
   ASSERT_EQ(1, lru_replacer.Size());
-  lru_replacer.Evict(&value);
-  ASSERT_EQ(value, 1);
+  ASSERT_EQ(1, lru_replacer.Evict());
   ASSERT_EQ(0, lru_replacer.Size());
 
-  // This operation should not modify size
-  ASSERT_EQ(false, lru_replacer.Evict(&value));
+  // There is nothing left in the replacer, so make sure this doesn't do something strange.
+  frame = lru_replacer.Evict();
+  ASSERT_EQ(false, frame.has_value());
   ASSERT_EQ(0, lru_replacer.Size());
+
+  // Make sure that setting a non-existent frame as evictable or non-evictable doesn't do something strange.
+  lru_replacer.SetEvictable(6, false);
+  lru_replacer.SetEvictable(6, true);
 }
+
 }  // namespace bustub