Skip to content

Commit

Permalink
Use DefaultServices for server_test (#4566)
Browse files Browse the repository at this point in the history
<!-- Describe what has changed in this PR -->
**What changed?**
1. We now use `ForServices(DefaultServices)` in the server test. 
2. In addition, we only check error messages while the server is
starting and running, not while it is shutting down.

<!-- Tell your future self why have you made these changes -->
**Why?**
1. This test wasn't doing much without ForServices, which I mistakenly
removed in a previous PR. It was just starting the server but no
services.
2. The volume and variety of errors during shutdown is intractable

<!-- How have you verified this change? Tested locally? Added a unit
test? Checked in staging env? -->
**How did you test it?**
I ran it locally 10 times, and twice on the CI.

<!-- Assuming the worst case, what can be broken when deploying this
change to production? -->
**Potential risks**
Could easily become a flaky test.

<!-- Is this PR a hotfix candidate or require that a notification be
sent to the broader community? (Yes/No) -->
**Is hotfix candidate?**
No
  • Loading branch information
MichaelSnowden committed Jul 5, 2023
1 parent db8b67a commit 17259db
Showing 1 changed file with 90 additions and 34 deletions.
124 changes: 90 additions & 34 deletions temporal/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,21 +22,23 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.

package temporal
package temporal_test

import (
"fmt"
"path"
"strings"
"sync/atomic"
"testing"
"time"

"go.temporal.io/server/common/config"
"go.temporal.io/server/common/log"
"go.temporal.io/server/common/log/tag"
_ "go.temporal.io/server/common/persistence/sql/sqlplugin/sqlite" // needed to register the sqlite plugin
"go.temporal.io/server/temporal"
"go.temporal.io/server/tests/testutils"

"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand All @@ -46,17 +48,18 @@ import (
func TestNewServer(t *testing.T) {
t.Parallel()

ctrl := gomock.NewController(t)

cfg := loadConfig(t)
logger := newErrorLogDetector(t, ctrl)
logDetector := newErrorLogDetector(t)
logDetector.Start()

server, err := NewServer(
WithConfig(cfg),
WithLogger(logger),
server, err := temporal.NewServer(
temporal.ForServices(temporal.DefaultServices),
temporal.WithConfig(cfg),
temporal.WithLogger(logDetector),
)
require.NoError(t, err)
t.Cleanup(func() {
logDetector.Stop()
assert.NoError(t, server.Stop())
})
require.NoError(t, server.Start())
Expand Down Expand Up @@ -98,35 +101,88 @@ func setTestPorts(cfg *config.Config) {
}
}

type errorLogDetector struct {
t testing.TB
on atomic.Bool
log.Logger
}

func (d *errorLogDetector) Start() {
d.on.Store(true)
}

func (d *errorLogDetector) Stop() {
d.on.Store(false)
}

func (d *errorLogDetector) Warn(msg string, tags ...tag.Tag) {
d.Logger.Warn(msg, tags...)

if !d.on.Load() {
return
}

if strings.Contains(msg, "error creating sdk client") {
return
}

d.t.Errorf("unexpected warning log: %s", msg)
}

func (d *errorLogDetector) Error(msg string, tags ...tag.Tag) {
d.Logger.Error(msg, tags...)

if !d.on.Load() {
return
}

if strings.Contains(msg, "Unable to process new range") {
return
}

d.t.Errorf("unexpected error log: %s", msg)
}

// newErrorLogDetector returns a logger that fails the test if it logs any errors or warnings, except for the ones that
// are expected. Ideally, there are no "expected" errors or warnings, but we still want this test to avoid introducing
// any new ones while we are working on removing the existing ones.
func newErrorLogDetector(t *testing.T, ctrl *gomock.Controller) *log.MockLogger {
logger := log.NewMockLogger(ctrl)
logger.EXPECT().Debug(gomock.Any(), gomock.Any()).AnyTimes()
logger.EXPECT().Info(gomock.Any(), gomock.Any()).AnyTimes()
logger.EXPECT().Warn(gomock.Any(), gomock.Any()).AnyTimes().Do(func(msg string, tags ...tag.Tag) {
if strings.Contains(msg, "error creating sdk client") {
return
}

if strings.Contains(msg, "poll for task") {
return
}

if strings.Contains(msg, "error in prometheus") {
return
}

t.Errorf("unexpected warning: %v", msg)
})
logger.EXPECT().Error(gomock.Any(), gomock.Any()).AnyTimes().Do(func(msg string, tags ...tag.Tag) {
if strings.Contains(msg, "looking up host for shardID") {
return
}
func newErrorLogDetector(t testing.TB) *errorLogDetector {
return &errorLogDetector{
t: t,
Logger: log.NewCLILogger(),
}
}

t.Errorf("unexpected error: %v", msg)
})
type fakeTest struct {
testing.TB
errorfMsgs []string
}

func (f *fakeTest) Errorf(msg string, args ...any) {
f.errorfMsgs = append(f.errorfMsgs, fmt.Sprintf(msg, args...))
}

func TestErrorLogDetector(t *testing.T) {
t.Parallel()

f := &fakeTest{TB: t}
d := newErrorLogDetector(f)
d.Start()
d.Warn("error creating sdk client")
d.Error("Unable to process new range")
d.Error("unexpected error")
d.Warn("unexpected warning")

assert.Equal(t, []string{
"unexpected error log: unexpected error",
"unexpected warning log: unexpected warning",
}, f.errorfMsgs, "should fail the test if there are any unexpected errors or warnings")

d.Stop()

f.errorfMsgs = nil

return logger
d.Error("unexpected error")
d.Warn("unexpected warning")
assert.Empty(t, f.errorfMsgs, "should not fail the test if the detector is stopped")
}

0 comments on commit 17259db

Please sign in to comment.