From 2bc6cf65cae37c3b07e5af784819a852043bdf0d Mon Sep 17 00:00:00 2001
From: Faye Amacker <33205765+fxamacker@users.noreply.github.com>
Date: Wed, 2 Oct 2024 14:01:06 -0500
Subject: [PATCH] Prevent migration tests from using out-of-sync data

Currently, some migration tests reuse storage for migration.
It can lead to tests failing when the storage cache gets
out of sync with underlying storage.

For example:
1. Use storage 1 to store valueA in public domain directly.
2. Use storage 2 to store valueB in public domain via
   ExecuteTransaction().
3. Reuse storage 1 for migration.  Here, migration
   only sees valueA from cache instead of both values.

This commit prevents out-of-sync issues by creating new
runtime.Storage for migrations instead of reusing old storage.
---
 migrations/capcons/migration_test.go | 85 +++++++++++++++++++---------
 1 file changed, 58 insertions(+), 27 deletions(-)

diff --git a/migrations/capcons/migration_test.go b/migrations/capcons/migration_test.go
index 1a136d01b..86e0dfa20 100644
--- a/migrations/capcons/migration_test.go
+++ b/migrations/capcons/migration_test.go
@@ -511,18 +511,20 @@ func testPathCapabilityValueMigration(
 	)
 
 	// Create and store path and account links
+	{
+		// Create new runtime.Storage used for storing path and account links
+		storage, inter, err := rt.Storage(runtime.Context{
+			Interface: runtimeInterface,
+		})
+		require.NoError(t, err)
 
-	storage, inter, err := rt.Storage(runtime.Context{
-		Interface: runtimeInterface,
-	})
-	require.NoError(t, err)
+		storeTestPathLinks(t, pathLinks, storage, inter)
 
-	storeTestPathLinks(t, pathLinks, storage, inter)
+		storeTestAccountLinks(accountLinks, storage, inter)
 
-	storeTestAccountLinks(accountLinks, storage, inter)
-
-	err = storage.Commit(inter, false)
-	require.NoError(t, err)
+		err = storage.Commit(inter, false)
+		require.NoError(t, err)
+	}
 
 	// Save capability values into account
 
@@ -557,6 +559,15 @@ func testPathCapabilityValueMigration(
 
 	// Migrate
 
+	// Create new runtime.Storage for migration.
+	// WARNING: don't reuse old storage (created for storing path and account links)
+	// because it has outdated cache after ExecuteTransaction() commits new data
+	// to underlying ledger.
+	storage, inter, err := rt.Storage(runtime.Context{
+		Interface: runtimeInterface,
+	})
+	require.NoError(t, err)
+
 	migration, err := migrations.NewStorageMigration(inter, storage, "test", testAddress)
 	require.NoError(t, err)
 
@@ -2410,16 +2421,17 @@ func TestPublishedPathCapabilityValueMigration(t *testing.T) {
 	)
 
 	// Create and store path links
+	{
+		storage, inter, err := rt.Storage(runtime.Context{
+			Interface: runtimeInterface,
+		})
+		require.NoError(t, err)
 
-	storage, inter, err := rt.Storage(runtime.Context{
-		Interface: runtimeInterface,
-	})
-	require.NoError(t, err)
-
-	storeTestPathLinks(t, pathLinks, storage, inter)
+		storeTestPathLinks(t, pathLinks, storage, inter)
 
-	err = storage.Commit(inter, false)
-	require.NoError(t, err)
+		err = storage.Commit(inter, false)
+		require.NoError(t, err)
+	}
 
 	// Save capability values into account
 
@@ -2432,7 +2444,7 @@ func TestPublishedPathCapabilityValueMigration(t *testing.T) {
       }
     `
 
-	err = rt.ExecuteTransaction(
+	err := rt.ExecuteTransaction(
 		runtime.Script{
 			Source: []byte(setupTx),
 		},
@@ -2446,6 +2458,15 @@ func TestPublishedPathCapabilityValueMigration(t *testing.T) {
 
 	// Migrate
 
+	// Create new runtime.Storage for migration.
+	// WARNING: don't reuse old storage (created for storing path links)
+	// because it has outdated cache after ExecuteTransaction() commits new data
+	// to underlying ledger.
+	storage, inter, err := rt.Storage(runtime.Context{
+		Interface: runtimeInterface,
+	})
+	require.NoError(t, err)
+
 	migration, err := migrations.NewStorageMigration(inter, storage, "test", testAddress)
 	require.NoError(t, err)
 
@@ -2663,16 +2684,17 @@ func TestUntypedPathCapabilityValueMigration(t *testing.T) {
 	)
 
 	// Create and store path links
+	{
+		storage, inter, err := rt.Storage(runtime.Context{
+			Interface: runtimeInterface,
+		})
+		require.NoError(t, err)
 
-	storage, inter, err := rt.Storage(runtime.Context{
-		Interface: runtimeInterface,
-	})
-	require.NoError(t, err)
-
-	storeTestPathLinks(t, pathLinks, storage, inter)
+		storeTestPathLinks(t, pathLinks, storage, inter)
 
-	err = storage.Commit(inter, false)
-	require.NoError(t, err)
+		err = storage.Commit(inter, false)
+		require.NoError(t, err)
+	}
 
 	// Save capability values into account
 
@@ -2686,7 +2708,7 @@ func TestUntypedPathCapabilityValueMigration(t *testing.T) {
       }
     `
 
-	err = rt.ExecuteTransaction(
+	err := rt.ExecuteTransaction(
 		runtime.Script{
 			Source: []byte(setupTx),
 		},
@@ -2700,6 +2722,15 @@ func TestUntypedPathCapabilityValueMigration(t *testing.T) {
 
 	// Migrate
 
+	// Create new runtime.Storage for migration.
+	// WARNING: don't reuse old storage (created for storing path links)
+	// because it has outdated cache after ExecuteTransaction() commits new data
+	// to underlying ledger.
+	storage, inter, err := rt.Storage(runtime.Context{
+		Interface: runtimeInterface,
+	})
+	require.NoError(t, err)
+
 	migration, err := migrations.NewStorageMigration(inter, storage, "test", testAddress)
 	require.NoError(t, err)