[GitHub] [beam] damondouglas commented on a diff in pull request #25791: [Playground] Start Google Cloud Datastore emulator from Go tests automatically

["damondouglas (via GitHub)" <gi...@apache.org>]

damondouglas commented on code in PR #25791:
URL: https://github.com/apache/beam/pull/25791#discussion_r1163392187


##########
playground/backend/internal/db/schema/migration/migrations_test.go:
##########
@@ -61,8 +57,9 @@ func setup() {
 }
 
 func teardown() {
-	if err := datastoreDb.Client.Close(); err != nil {
-		panic(err)
+	clientCloseErr := datastoreDb.Close()
+	if clientCloseErr != nil {
+		panic(clientCloseErr)

Review Comment:
   Perhaps these errors could be collected in chan error and handled like:
   
   ```
   for {
      select {
         case err := <- clientErr:
              // gracefully shut down other processes
              return err
        case err := <- datastoreDbErr:
              // gracefully shut down other processes
              return err
      }
   }
   ```



##########
playground/backend/internal/db/datastore/emulator_wrapper.go:
##########
@@ -0,0 +1,182 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements.  See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License.  You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package datastore
+
+import (
+	"beam.apache.org/playground/backend/internal/constants"
+	"beam.apache.org/playground/backend/internal/db/mapper"
+	"beam.apache.org/playground/backend/internal/logger"
+	"context"
+	"fmt"
+	"math/rand"
+	"net"
+	"os"
+	"os/exec"
+	"syscall"
+	"time"
+)
+
+const (
+	emulatorHost    = "127.0.0.1"
+	portRangeStart  = 10000
+	portRangeEnd    = 20000
+	maxAttempts     = 3
+	startupDuration = 500 * time.Millisecond
+	pauseDuration   = 100 * time.Millisecond
+	waitDuration    = 10 * time.Second
+)
+
+type EmulatedDatastore struct {
+	*Datastore
+	emulator *emulator
+}
+
+type emulator struct {
+	Host string
+	Port int
+	cmd  *exec.Cmd
+}
+
+func (ed EmulatedDatastore) Close() error {
+	clientCloseErr := ed.Datastore.Client.Close()
+	emulatorStopErr := ed.emulator.Stop()
+	if clientCloseErr != nil {
+		return clientCloseErr
+	}
+	if emulatorStopErr != nil {
+		return emulatorStopErr
+	}
+	return nil
+}
+
+func NewEmulated(ctx context.Context) (EmulatedDatastore, error) {
+	emulator, err := startEmulator()
+	if err != nil {
+		return EmulatedDatastore{}, err
+	}
+
+	if _, ok := os.LookupEnv("GOOGLE_CLOUD_PROJECT"); ok {
+		err := os.Unsetenv("GOOGLE_CLOUD_PROJECT")
+		if err != nil {
+			panic(err)
+		}
+	}
+
+	if err := os.Setenv(constants.EmulatorHostKey, emulator.GetEndpoint()); err != nil {
+		panic(err)
+	}
+
+	datastoreDb, err := New(ctx, mapper.NewPrecompiledObjectMapper(), constants.EmulatorProjectId)
+	if err != nil {
+		return EmulatedDatastore{}, err
+	}
+
+	return EmulatedDatastore{Datastore: datastoreDb, emulator: emulator}, nil
+}
+
+func startEmulator() (*emulator, error) {

Review Comment:
   It may make your code cleaner and easy to troubleshoot if you receive a `ctx context.Context` and use the pattern:
   ```
   ctx, cancel := context.WithCancel(ctx)
   defer cancel()
   ```
   
   The test could perhaps do:
   ```
   ctx, cancel := context.WithTimeout(...)
   ```
   
   Would you like me to create an example?  Please let me know.



##########
playground/backend/internal/db/datastore/emulator_wrapper.go:
##########
@@ -0,0 +1,182 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements.  See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License.  You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package datastore
+
+import (
+	"beam.apache.org/playground/backend/internal/constants"
+	"beam.apache.org/playground/backend/internal/db/mapper"
+	"beam.apache.org/playground/backend/internal/logger"
+	"context"
+	"fmt"
+	"math/rand"
+	"net"
+	"os"
+	"os/exec"
+	"syscall"
+	"time"
+)
+
+const (
+	emulatorHost    = "127.0.0.1"
+	portRangeStart  = 10000
+	portRangeEnd    = 20000
+	maxAttempts     = 3
+	startupDuration = 500 * time.Millisecond
+	pauseDuration   = 100 * time.Millisecond
+	waitDuration    = 10 * time.Second
+)
+
+type EmulatedDatastore struct {
+	*Datastore
+	emulator *emulator
+}
+
+type emulator struct {
+	Host string
+	Port int
+	cmd  *exec.Cmd
+}
+
+func (ed EmulatedDatastore) Close() error {
+	clientCloseErr := ed.Datastore.Client.Close()
+	emulatorStopErr := ed.emulator.Stop()

Review Comment:
   Usually in go it makes sense to evaluate one error at a time.  I wonder if it makes sense to instead call Client.Close and emulator.Stop within the test code itself, or some shared teardown method used by all the tests.



##########
playground/backend/internal/db/datastore/emulator_wrapper.go:
##########
@@ -0,0 +1,182 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements.  See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License.  You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package datastore
+
+import (
+	"beam.apache.org/playground/backend/internal/constants"
+	"beam.apache.org/playground/backend/internal/db/mapper"
+	"beam.apache.org/playground/backend/internal/logger"
+	"context"
+	"fmt"
+	"math/rand"
+	"net"
+	"os"
+	"os/exec"
+	"syscall"
+	"time"
+)
+
+const (
+	emulatorHost    = "127.0.0.1"
+	portRangeStart  = 10000
+	portRangeEnd    = 20000
+	maxAttempts     = 3
+	startupDuration = 500 * time.Millisecond
+	pauseDuration   = 100 * time.Millisecond
+	waitDuration    = 10 * time.Second
+)
+
+type EmulatedDatastore struct {
+	*Datastore
+	emulator *emulator
+}
+
+type emulator struct {
+	Host string
+	Port int
+	cmd  *exec.Cmd
+}
+
+func (ed EmulatedDatastore) Close() error {
+	clientCloseErr := ed.Datastore.Client.Close()
+	emulatorStopErr := ed.emulator.Stop()
+	if clientCloseErr != nil {
+		return clientCloseErr
+	}
+	if emulatorStopErr != nil {
+		return emulatorStopErr
+	}
+	return nil
+}
+
+func NewEmulated(ctx context.Context) (EmulatedDatastore, error) {
+	emulator, err := startEmulator()
+	if err != nil {
+		return EmulatedDatastore{}, err
+	}
+
+	if _, ok := os.LookupEnv("GOOGLE_CLOUD_PROJECT"); ok {
+		err := os.Unsetenv("GOOGLE_CLOUD_PROJECT")
+		if err != nil {
+			panic(err)
+		}
+	}
+
+	if err := os.Setenv(constants.EmulatorHostKey, emulator.GetEndpoint()); err != nil {
+		panic(err)
+	}
+
+	datastoreDb, err := New(ctx, mapper.NewPrecompiledObjectMapper(), constants.EmulatorProjectId)
+	if err != nil {
+		return EmulatedDatastore{}, err

Review Comment:
   Perhaps return a pointer instead of a struct so that the code follows the convention: `return nil, err`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org