Redo the device data unit tests

They were A) confusing and B) testing the wrong thing as they got refactored
after the bad refactor of the source code..

state/device_data_table_test.go

@@ -22,17 +22,20 @@ func assertDeviceData(t *testing.T, g, w internal.DeviceData) {
 	assertVal(t, "FallbackKeyTypes", g.FallbackKeyTypes, w.FallbackKeyTypes)
 	assertVal(t, "OTKCounts", g.OTKCounts, w.OTKCounts)
 	assertVal(t, "ChangedBits", g.ChangedBits, w.ChangedBits)
-	assertVal(t, "DeviceLists", g.DeviceLists, w.DeviceLists)
+	if w.DeviceLists.Sent != nil {
+		assertVal(t, "DeviceLists.Sent", g.DeviceLists.Sent, w.DeviceLists.Sent)
+	}
 }
 
-func TestDeviceDataTableSwaps(t *testing.T) {
+// Tests OTKCounts and FallbackKeyTypes behaviour
+func TestDeviceDataTableOTKCountAndFallbackKeyTypes(t *testing.T) {
 	db, close := connectToDB(t)
 	defer close()
 	table := NewDeviceDataTable(db)
-	userID := "@bob"
+	userID := "@TestDeviceDataTableOTKCountAndFallbackKeyTypes"
 	deviceID := "BOB"
 
-	// test accumulating deltas
+	// these are individual updates from Synapse from /sync v2
 	deltas := []internal.DeviceData{
 		{
 			UserID:   userID,
@@ -46,9 +49,6 @@ func TestDeviceDataTableSwaps(t *testing.T) {
 			UserID:           userID,
 			DeviceID:         deviceID,
 			FallbackKeyTypes: []string{"foobar"},
-			DeviceLists: internal.DeviceLists{
-				New: internal.ToDeviceListChangesMap([]string{"alice"}, nil),
-			},
 		},
 		{
 			UserID:   userID,
@@ -60,16 +60,38 @@ func TestDeviceDataTableSwaps(t *testing.T) {
 		{
 			UserID:   userID,
 			DeviceID: deviceID,
-			DeviceLists: internal.DeviceLists{
-				New: internal.ToDeviceListChangesMap([]string{"💣"}, nil),
-			},
 		},
 	}
+
+	// apply them
 	for _, dd := range deltas {
 		err := table.Upsert(&dd)
 		assertNoError(t, err)
 	}
 
+	// read them without swap, it should have replaced them correctly.
+	// Because sync v2 sends the complete OTK count and complete fallback key types
+	// every time, we always use the latest values. Because we aren't swapping, repeated
+	// reads produce the same result.
+	for i := 0; i < 3; i++ {
+		got, err := table.Select(userID, deviceID, false)
+		mustNotError(t, err)
+		want := internal.DeviceData{
+			UserID:   userID,
+			DeviceID: deviceID,
+			OTKCounts: map[string]int{
+				"foo": 99,
+			},
+			FallbackKeyTypes: []string{"foobar"},
+		}
+		want.SetFallbackKeysChanged()
+		want.SetOTKCountChanged()
+		assertDeviceData(t, *got, want)
+	}
+	// now we swap the data. This still returns the same values, but the changed bits are no longer set
+	// on subsequent reads.
+	got, err := table.Select(userID, deviceID, true)
+	mustNotError(t, err)
 	want := internal.DeviceData{
 		UserID:   userID,
 		DeviceID: deviceID,
@@ -77,68 +99,118 @@ func TestDeviceDataTableSwaps(t *testing.T) {
 			"foo": 99,
 		},
 		FallbackKeyTypes: []string{"foobar"},
-		DeviceLists: internal.DeviceLists{
-			New:  internal.ToDeviceListChangesMap([]string{"alice", "💣"}, nil),
-			Sent: map[string]int{},
-		},
 	}
 	want.SetFallbackKeysChanged()
 	want.SetOTKCountChanged()
-	// check we can read-only select
+	assertDeviceData(t, *got, want)
+
+	// subsequent read
+	got, err = table.Select(userID, deviceID, false)
+	mustNotError(t, err)
+	want = internal.DeviceData{
+		UserID:   userID,
+		DeviceID: deviceID,
+		OTKCounts: map[string]int{
+			"foo": 99,
+		},
+		FallbackKeyTypes: []string{"foobar"},
+	}
+	assertDeviceData(t, *got, want)
+}
+
+// Tests the DeviceLists field
+func TestDeviceDataTableDeviceList(t *testing.T) {
+	db, close := connectToDB(t)
+	defer close()
+	table := NewDeviceDataTable(db)
+	userID := "@TestDeviceDataTableDeviceList"
+	deviceID := "BOB"
+
+	// these are individual updates from Synapse from /sync v2
+	deltas := []internal.DeviceData{
+		{
+			UserID:   userID,
+			DeviceID: deviceID,
+			DeviceLists: internal.DeviceLists{
+				New: internal.ToDeviceListChangesMap([]string{"alice"}, nil),
+			},
+		},
+		{
+			UserID:   userID,
+			DeviceID: deviceID,
+			DeviceLists: internal.DeviceLists{
+				New: internal.ToDeviceListChangesMap([]string{"💣"}, nil),
+			},
+		},
+	}
+	// apply them
+	for _, dd := range deltas {
+		err := table.Upsert(&dd)
+		assertNoError(t, err)
+	}
+
+	// check we can read-only select. This doesn't modify any fields.
 	for i := 0; i < 3; i++ {
 		got, err := table.Select(userID, deviceID, false)
 		assertNoError(t, err)
-		assertDeviceData(t, *got, want)
+		assertDeviceData(t, *got, internal.DeviceData{
+			UserID:   userID,
+			DeviceID: deviceID,
+			DeviceLists: internal.DeviceLists{
+				Sent: internal.MapStringInt{}, // until we "swap" we don't consume the New entries
+			},
+		})
 	}
-	// now swap-er-roo, at this point we still expect the "old" data,
-	// as it is the first time we swap
+	// now swap-er-roo, which shifts everything from New into Sent.
 	got, err := table.Select(userID, deviceID, true)
 	assertNoError(t, err)
-	assertDeviceData(t, *got, want)
-
-	// changed bits were reset when we swapped
-	want2 := want
-	want2.DeviceLists = internal.DeviceLists{
-		Sent: internal.ToDeviceListChangesMap([]string{"alice", "💣"}, nil),
-		New:  map[string]int{},
-	}
-	want2.ChangedBits = 0
-	want.ChangedBits = 0
+	assertDeviceData(t, *got, internal.DeviceData{
+		UserID:   userID,
+		DeviceID: deviceID,
+		DeviceLists: internal.DeviceLists{
+			Sent: internal.ToDeviceListChangesMap([]string{"alice", "💣"}, nil),
+		},
+	})
 
 	// this is permanent, read-only views show this too.
-	// Since we have swapped previously, we now expect New to be empty
-	// and Sent to be set. Swap again to clear Sent.
-	got, err = table.Select(userID, deviceID, true)
-	assertNoError(t, err)
-	assertDeviceData(t, *got, want2)
-
-	// We now expect empty DeviceLists, as we swapped twice.
 	got, err = table.Select(userID, deviceID, false)
 	assertNoError(t, err)
-	want3 := want2
-	want3.DeviceLists = internal.DeviceLists{
-		Sent: map[string]int{},
-		New:  map[string]int{},
-	}
-	assertDeviceData(t, *got, want3)
+	assertDeviceData(t, *got, internal.DeviceData{
+		UserID:   userID,
+		DeviceID: deviceID,
+		DeviceLists: internal.DeviceLists{
+			Sent: internal.ToDeviceListChangesMap([]string{"alice", "💣"}, nil),
+		},
+	})
+
+	// We now expect empty DeviceLists, as we swapped twice.
+	got, err = table.Select(userID, deviceID, true)
+	assertNoError(t, err)
+	assertDeviceData(t, *got, internal.DeviceData{
+		UserID:   userID,
+		DeviceID: deviceID,
+		DeviceLists: internal.DeviceLists{
+			Sent: internal.MapStringInt{},
+		},
+	})
 
 	// get back the original state
-	//err = table.DeleteDevice(userID, deviceID)
 	assertNoError(t, err)
 	for _, dd := range deltas {
 		err = table.Upsert(&dd)
 		assertNoError(t, err)
 	}
-	want.SetFallbackKeysChanged()
-	want.SetOTKCountChanged()
-	got, err = table.Select(userID, deviceID, false)
-	assertNoError(t, err)
-	assertDeviceData(t, *got, want)
-
-	// swap once then add once so both sent and new are populated
-	// Moves Alice and Bob to Sent
-	_, err = table.Select(userID, deviceID, true)
+	// Move original state to Sent by swapping
+	got, err = table.Select(userID, deviceID, true)
 	assertNoError(t, err)
+	assertDeviceData(t, *got, internal.DeviceData{
+		UserID:   userID,
+		DeviceID: deviceID,
+		DeviceLists: internal.DeviceLists{
+			Sent: internal.ToDeviceListChangesMap([]string{"alice", "💣"}, nil),
+		},
+	})
+	// Add new entries to New before acknowledging Sent
 	err = table.Upsert(&internal.DeviceData{
 		UserID:   userID,
 		DeviceID: deviceID,
@@ -148,20 +220,18 @@ func TestDeviceDataTableSwaps(t *testing.T) {
 	})
 	assertNoError(t, err)
 
-	want.ChangedBits = 0
-
-	want4 := want
-	want4.DeviceLists = internal.DeviceLists{
-		Sent: internal.ToDeviceListChangesMap([]string{"alice", "💣"}, nil),
-		New:  internal.ToDeviceListChangesMap([]string{"💣"}, []string{"charlie"}),
-	}
-	// Without swapping, we expect Alice and Bob in Sent, and Bob and Charlie in New
+	// Reading without swapping does not move New->Sent, so returns the previous value
 	got, err = table.Select(userID, deviceID, false)
 	assertNoError(t, err)
-	assertDeviceData(t, *got, want4)
+	assertDeviceData(t, *got, internal.DeviceData{
+		UserID:   userID,
+		DeviceID: deviceID,
+		DeviceLists: internal.DeviceLists{
+			Sent: internal.ToDeviceListChangesMap([]string{"alice", "💣"}, nil),
+		},
+	})
 
-	// another append then consume
-	// This results in dave to be added to New
+	// Append even more items to New
 	err = table.Upsert(&internal.DeviceData{
 		UserID:   userID,
 		DeviceID: deviceID,
@@ -170,24 +240,28 @@ func TestDeviceDataTableSwaps(t *testing.T) {
 		},
 	})
 	assertNoError(t, err)
-	got, err = table.Select(userID, deviceID, true)
-	assertNoError(t, err)
-	want5 := want4
-	want5.DeviceLists = internal.DeviceLists{
-		Sent: internal.ToDeviceListChangesMap([]string{"alice", "💣"}, nil),
-		New:  internal.ToDeviceListChangesMap([]string{"💣"}, []string{"charlie", "dave"}),
-	}
-	assertDeviceData(t, *got, want5)
 
-	// Swapping again clears New
+	// Now swap: all the combined items in New go into Sent
 	got, err = table.Select(userID, deviceID, true)
 	assertNoError(t, err)
-	want5 = want4
-	want5.DeviceLists = internal.DeviceLists{
-		Sent: internal.ToDeviceListChangesMap([]string{"💣"}, []string{"charlie", "dave"}),
-		New:  map[string]int{},
-	}
-	assertDeviceData(t, *got, want5)
+	assertDeviceData(t, *got, internal.DeviceData{
+		UserID:   userID,
+		DeviceID: deviceID,
+		DeviceLists: internal.DeviceLists{
+			Sent: internal.ToDeviceListChangesMap([]string{"💣", "dave"}, []string{"charlie", "dave"}),
+		},
+	})
+
+	// Swapping again clears Sent out, and since nothing is in New we get an empty list
+	got, err = table.Select(userID, deviceID, true)
+	assertNoError(t, err)
+	assertDeviceData(t, *got, internal.DeviceData{
+		UserID:   userID,
+		DeviceID: deviceID,
+		DeviceLists: internal.DeviceLists{
+			Sent: internal.MapStringInt{},
+		},
+	})
 
 	// delete everything, no data returned
 	assertNoError(t, table.DeleteDevice(userID, deviceID))