Fix a couple race conditions in tests (#689)

* Fix race condition on `LoopSuite`
* Fix race condition in the access to a Otto.Value in tests (quick fix, need to improve approach in the future)

geth/common/types.go

@@ -177,6 +177,8 @@ type JailCell interface {
 	Set(string, interface{}) error
 	// Get a value from VM.
 	Get(string) (otto.Value, error)
+	// GetObjectValue returns the given name's otto.Value from the given otto.Value v. Should only be needed in tests.
+	GetObjectValue(otto.Value, string) (otto.Value, error)
 	// Run an arbitrary JS code. Input maybe string or otto.Script.
 	Run(interface{}) (otto.Value, error)
 	// Call an arbitrary JS function by name and args.

geth/common/types_mock.go

@@ -431,6 +431,19 @@ func (mr *MockJailCellMockRecorder) Get(arg0 interface{}) *gomock.Call {
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockJailCell)(nil).Get), arg0)
 }
 
+// GetObjectValue mocks base method
+func (m *MockJailCell) GetObjectValue(arg0 otto.Value, arg1 string) (otto.Value, error) {
+	ret := m.ctrl.Call(m, "GetObjectValue", arg0, arg1)
+	ret0, _ := ret[0].(otto.Value)
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
+}
+
+// GetObjectValue indicates an expected call of GetObjectValue
+func (mr *MockJailCellMockRecorder) GetObjectValue(arg0, arg1 interface{}) *gomock.Call {
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetObjectValue", reflect.TypeOf((*MockJailCell)(nil).GetObjectValue), arg0, arg1)
+}
+
 // Run mocks base method
 func (m *MockJailCell) Run(arg0 interface{}) (otto.Value, error) {
 	ret := m.ctrl.Call(m, "Run", arg0)

geth/jail/cell_test.go

@@ -170,10 +170,10 @@ func (s *CellTestSuite) TestCellFetchErrorRace() {
 	case <-dataCh:
 		s.Fail("fetch didn't return error for nonexistent url")
 	case e := <-errCh:
-		name, err := e.Object().Get("name")
+		name, err := cell.GetObjectValue(e, "name")
 		s.NoError(err)
 		s.Equal("Error", name.String())
-		_, err = e.Object().Get("message")
+		_, err = cell.GetObjectValue(e, "message")
 		s.NoError(err)
 	case <-time.After(5 * time.Second):
 		s.Fail("test timed out")

geth/jail/handlers_test.go

@@ -9,12 +9,13 @@ import (
 
 	"github.com/robertkrimen/otto"
 
+	"sync/atomic"
+
 	gethrpc "github.com/ethereum/go-ethereum/rpc"
 	"github.com/status-im/status-go/geth/params"
 	"github.com/status-im/status-go/geth/rpc"
 	"github.com/status-im/status-go/geth/signal"
 	"github.com/stretchr/testify/suite"
-	"sync/atomic"
 )
 
 func TestHandlersTestSuite(t *testing.T) {
@@ -179,7 +180,7 @@ func (s *HandlersTestSuite) TestSendSignalHandler() {
 
 	value, err := cell.Run(`statusSignals.sendSignal("test signal message")`)
 	s.NoError(err)
-	result, err := value.Object().Get("result")
+	result, err := cell.GetObjectValue(value, "result")
 	s.NoError(err)
 	resultBool, err := result.ToBoolean()
 	s.NoError(err)

geth/jail/internal/loop/loop_test.go

@@ -54,8 +54,9 @@ func (s *LoopSuite) SetupTest() {
 	ctx, cancel := context.WithCancel(context.Background())
 	s.cancel = cancel
 	go func() {
+		a := s.Assertions // Cache assertions reference as otherwise we'd incur in a race condition
 		err := s.loop.Run(ctx)
-		s.Equal(context.Canceled, err)
+		a.Equal(context.Canceled, err)
 	}()
 }
 

geth/jail/internal/vm/vm.go

@@ -34,7 +34,7 @@ func (vm *VM) Set(key string, val interface{}) error {
 	return vm.vm.Set(key, val)
 }
 
-// Get returns the giving key's otto.Value from the underline otto vm.
+// Get returns the given key's otto.Value from the underlying otto vm.
 func (vm *VM) Get(key string) (otto.Value, error) {
 	vm.Lock()
 	defer vm.Unlock()
@@ -42,6 +42,14 @@ func (vm *VM) Get(key string) (otto.Value, error) {
 	return vm.vm.Get(key)
 }
 
+// GetObjectValue returns the given name's otto.Value from the given otto.Value v. Should only be needed in tests.
+func (vm *VM) GetObjectValue(v otto.Value, name string) (otto.Value, error) {
+	vm.Lock()
+	defer vm.Unlock()
+
+	return v.Object().Get(name)
+}
+
 // Call attempts to call the internal call function for the giving response associated with the
 // proper values.
 func (vm *VM) Call(item string, this interface{}, args ...interface{}) (otto.Value, error) {

geth/jail/jail_test.go

@@ -194,3 +194,26 @@ func (s *JailTestSuite) TestExecute() {
 	`)
 	s.Equal(`{"test":true}`, response)
 }
+
+func (s *JailTestSuite) TestGetObjectValue() {
+	cell, result, err := s.Jail.createAndInitCell(
+		"cell1",
+		`var testCreateAndInitCell1 = {obj: 'objValue'}`,
+		`var testCreateAndInitCell2 = true`,
+		`testCreateAndInitCell2`,
+	)
+	s.NoError(err)
+	s.NotNil(cell)
+	s.Equal(`{"result":true}`, result)
+
+	testCreateAndInitCell1, err := cell.Get("testCreateAndInitCell1")
+	s.NoError(err)
+	s.True(testCreateAndInitCell1.IsObject())
+	value, err := cell.GetObjectValue(testCreateAndInitCell1, "obj")
+	s.NoError(err)
+	s.Equal("objValue", value.String())
+
+	value, err = cell.Get("testCreateAndInitCell2")
+	s.NoError(err)
+	s.Equal(`true`, value.String())
+}

t/e2e/whisper/whisper_jail_test.go

@@ -323,7 +323,7 @@ func (s *WhisperJailTestSuite) TestJailWhisper() {
 		for {
 			filter, err := cell.Get("filter")
 			r.NoError(err, "cannot get filter")
-			filterID, err := filter.Object().Get("filterId")
+			filterID, err := cell.GetObjectValue(filter, "filterId")
 			r.NoError(err, "cannot get filterId")
 
 			select {

t/e2e/whisper/whisper_mailbox_test.go

@@ -343,6 +343,7 @@ func (s *WhisperMailboxSuite) startMailboxBackend() (*api.StatusBackend, func())
 	mailboxConfig.WhisperConfig.DataDir = filepath.Join(datadir, "data")
 	mailboxConfig.DataDir = datadir
 
+	s.Require().False(mailboxBackend.IsNodeRunning())
 	s.Require().NoError(mailboxBackend.StartNode(mailboxConfig))
 	s.Require().True(mailboxBackend.IsNodeRunning())
 	return mailboxBackend, func() {