mirror of
https://github.com/mattermost/mattermost.git
synced 2026-04-27 17:21:09 -04:00
Some checks are pending
API / build (push) Waiting to run
Server CI / Compute Go Version (push) Waiting to run
Server CI / Check mocks (push) Blocked by required conditions
Server CI / Check go mod tidy (push) Blocked by required conditions
Server CI / check-style (push) Blocked by required conditions
Server CI / Check serialization methods for hot structs (push) Blocked by required conditions
Server CI / Vet API (push) Blocked by required conditions
Server CI / Check migration files (push) Blocked by required conditions
Server CI / Generate email templates (push) Blocked by required conditions
Server CI / Check store layers (push) Blocked by required conditions
Server CI / Check mmctl docs (push) Blocked by required conditions
Server CI / Postgres with binary parameters (push) Blocked by required conditions
Server CI / Postgres (push) Blocked by required conditions
Server CI / Postgres (FIPS) (push) Blocked by required conditions
Server CI / Generate Test Coverage (push) Blocked by required conditions
Server CI / Run mmctl tests (push) Blocked by required conditions
Server CI / Run mmctl tests (FIPS) (push) Blocked by required conditions
Server CI / Build mattermost server app (push) Blocked by required conditions
Web App CI / check-lint (push) Waiting to run
Web App CI / check-i18n (push) Waiting to run
Web App CI / check-types (push) Waiting to run
Web App CI / test (push) Waiting to run
Web App CI / build (push) Waiting to run
When we last bumped dependencies in https://github.com/mattermost/mattermost/pull/30005, `assert.NotSame` for maps started failing because of the change in https://github.com/stretchr/testify/issues/1661. The reality was that the previous assertion was silently skipped, and just now reporting as much. Here's an illustrative example: ```go package main import ( "maps" "testing" "github.com/stretchr/testify/assert" ) func TestClonedMapsAreNotSame(t *testing.T) { original := map[string]int{ "a": 1, "b": 2, "c": 3, } cloned := maps.Clone(original) assert.NotSame(t, original, cloned) } func TestSameMaps(t *testing.T) { original := map[string]int{ "a": 1, "b": 2, "c": 3, } cloned := original assert.Same(t, original, cloned) cloned["d"] = 4 assert.Same(t, original, cloned) } ``` which fails with the following after the original dependency update: ``` --- FAIL: TestClonedMapsAreNotSame (0.00s) main_test.go:19: Error Trace: /Users/jesse/tmp/testify/main_test.go:19 Error: Both arguments must be pointers Test: TestClonedMapsAreNotSame --- FAIL: TestSameMaps (0.00s) main_test.go:30: Error Trace: /Users/jesse/tmp/testify/main_test.go:30 Error: Both arguments must be pointers Test: TestSameMaps main_test.go:33: Error Trace: /Users/jesse/tmp/testify/main_test.go:33 Error: Both arguments must be pointers Test: TestSameMaps FAIL FAIL testassertequal 0.149s FAIL ``` However, instead of fixing the underlying issue, we took the address of those variables and kept using `assert.Same`. This isn't meaningful, since it doesn't directly compare the underlying pointers of the map objects in question, just the address of the pointers to those maps. Here's the output after taking the address (e.g. `&original` and `&cloned`): ``` --- FAIL: TestSameMaps (0.00s) main_test.go:30: Error Trace: /Users/jesse/tmp/testify/main_test.go:30 Error: Not same: expected: 0x14000070170 &map[string]int{"a":1, "b":2, "c":3} actual : 0x14000070178 &map[string]int{"a":1, "b":2, "c":3} Test: TestSameMaps main_test.go:33: Error Trace: /Users/jesse/tmp/testify/main_test.go:33 Error: Not same: expected: 0x14000070170 &map[string]int{"a":1, "b":2, "c":3, "d":4} actual : 0x14000070178 &map[string]int{"a":1, "b":2, "c":3, "d":4} Test: TestSameMaps FAIL FAIL testassertequal 0.157s FAIL ``` They are obviously the same map, since modifying `cloned` modified the original, yet `assert.Same` thinks they are different (because the pointe values are indeed different). (`assert.NotSame` "passes", but for the wrong reasons.) To fix this, introduce `model.AssertNotSameMap` to check this correctly.
207 lines
5.5 KiB
Go
207 lines
5.5 KiB
Go
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
|
|
// See LICENSE.txt for license information.
|
|
|
|
package platform
|
|
|
|
import (
|
|
"testing"
|
|
|
|
"github.com/mattermost/mattermost/server/public/model"
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
)
|
|
|
|
const broadcastTest = "test_broadcast_hook"
|
|
|
|
type testBroadcastHook struct{}
|
|
|
|
func (h *testBroadcastHook) Process(msg *HookedWebSocketEvent, webConn *WebConn, args map[string]any) error {
|
|
if args["makes_changes"].(bool) {
|
|
changesMade, _ := msg.Get("changes_made").(int)
|
|
msg.Add("changes_made", changesMade+1)
|
|
}
|
|
|
|
return nil
|
|
}
|
|
|
|
func TestRunBroadcastHooks(t *testing.T) {
|
|
mainHelper.Parallel(t)
|
|
hub := &Hub{
|
|
broadcastHooks: map[string]BroadcastHook{
|
|
broadcastTest: &testBroadcastHook{},
|
|
},
|
|
}
|
|
webConn := &WebConn{}
|
|
|
|
t.Run("should not allocate a new object when no hooks are passed", func(t *testing.T) {
|
|
event := model.NewWebSocketEvent(model.WebsocketEventPosted, "", "", "", nil, "")
|
|
|
|
result := hub.runBroadcastHooks(event, webConn, nil, nil)
|
|
|
|
assert.Same(t, event, result)
|
|
})
|
|
|
|
t.Run("should not allocate a new object when a hook is not making changes", func(t *testing.T) {
|
|
event := model.NewWebSocketEvent(model.WebsocketEventPosted, "", "", "", nil, "")
|
|
|
|
hookIDs := []string{
|
|
broadcastTest,
|
|
}
|
|
hookArgs := []map[string]any{
|
|
{
|
|
"makes_changes": false,
|
|
},
|
|
}
|
|
|
|
result := hub.runBroadcastHooks(event, webConn, hookIDs, hookArgs)
|
|
|
|
assert.Same(t, event, result)
|
|
})
|
|
|
|
t.Run("should allocate a new object and remove when a hook makes changes", func(t *testing.T) {
|
|
event := model.NewWebSocketEvent(model.WebsocketEventPosted, "", "", "", nil, "")
|
|
|
|
hookIDs := []string{
|
|
broadcastTest,
|
|
}
|
|
hookArgs := []map[string]any{
|
|
{
|
|
"makes_changes": true,
|
|
},
|
|
}
|
|
|
|
result := hub.runBroadcastHooks(event, webConn, hookIDs, hookArgs)
|
|
|
|
assert.NotSame(t, event, result)
|
|
model.AssertNotSameMap(t, event.GetData(), result.GetData())
|
|
assert.Equal(t, map[string]any{}, event.GetData())
|
|
assert.Equal(t, result.GetData(), map[string]any{
|
|
"changes_made": 1,
|
|
})
|
|
})
|
|
|
|
t.Run("should not allocate a new object when multiple hooks are not making changes", func(t *testing.T) {
|
|
event := model.NewWebSocketEvent(model.WebsocketEventPosted, "", "", "", nil, "")
|
|
|
|
hookIDs := []string{
|
|
broadcastTest,
|
|
broadcastTest,
|
|
broadcastTest,
|
|
}
|
|
hookArgs := []map[string]any{
|
|
{
|
|
"makes_changes": false,
|
|
},
|
|
{
|
|
"makes_changes": false,
|
|
},
|
|
{
|
|
"makes_changes": false,
|
|
},
|
|
}
|
|
|
|
result := hub.runBroadcastHooks(event, webConn, hookIDs, hookArgs)
|
|
|
|
assert.Same(t, event, result)
|
|
})
|
|
|
|
t.Run("should be able to make changes from only one of make hooks", func(t *testing.T) {
|
|
event := model.NewWebSocketEvent(model.WebsocketEventPosted, "", "", "", nil, "")
|
|
|
|
var hookIDs []string
|
|
var hookArgs []map[string]any
|
|
for i := range 10 {
|
|
hookIDs = append(hookIDs, broadcastTest)
|
|
hookArgs = append(hookArgs, map[string]any{
|
|
"makes_changes": i == 6,
|
|
})
|
|
}
|
|
|
|
result := hub.runBroadcastHooks(event, webConn, hookIDs, hookArgs)
|
|
|
|
assert.NotSame(t, event, result)
|
|
model.AssertNotSameMap(t, event.GetData(), result.GetData())
|
|
assert.Equal(t, event.GetData(), map[string]any{})
|
|
assert.Equal(t, result.GetData(), map[string]any{
|
|
"changes_made": 1,
|
|
})
|
|
})
|
|
|
|
t.Run("should be able to make changes from multiple hooks", func(t *testing.T) {
|
|
event := model.NewWebSocketEvent(model.WebsocketEventPosted, "", "", "", nil, "")
|
|
|
|
var hookIDs []string
|
|
var hookArgs []map[string]any
|
|
for range 10 {
|
|
hookIDs = append(hookIDs, broadcastTest)
|
|
hookArgs = append(hookArgs, map[string]any{
|
|
"makes_changes": true,
|
|
})
|
|
}
|
|
|
|
result := hub.runBroadcastHooks(event, webConn, hookIDs, hookArgs)
|
|
|
|
assert.NotSame(t, event, result)
|
|
model.AssertNotSameMap(t, event.GetData(), result.GetData())
|
|
assert.Equal(t, event.GetData(), map[string]any{})
|
|
assert.Equal(t, result.GetData(), map[string]any{
|
|
"changes_made": 10,
|
|
})
|
|
})
|
|
|
|
t.Run("should not remove precomputed JSON when a hook doesn't make changes", func(t *testing.T) {
|
|
event := model.NewWebSocketEvent(model.WebsocketEventPosted, "", "", "", nil, "")
|
|
event = event.PrecomputeJSON()
|
|
|
|
// Ensure that the event has precomputed JSON because changes aren't included when ToJSON is called again
|
|
originalJSON, _ := event.ToJSON()
|
|
event.Add("data", 1234)
|
|
eventJSON, _ := event.ToJSON()
|
|
require.Equal(t, string(originalJSON), string(eventJSON))
|
|
|
|
hookIDs := []string{
|
|
broadcastTest,
|
|
}
|
|
hookArgs := []map[string]any{
|
|
{
|
|
"makes_changes": false,
|
|
},
|
|
}
|
|
|
|
result := hub.runBroadcastHooks(event, webConn, hookIDs, hookArgs)
|
|
|
|
eventJSON, _ = event.ToJSON()
|
|
assert.Equal(t, string(originalJSON), string(eventJSON))
|
|
|
|
resultJSON, _ := result.ToJSON()
|
|
assert.Equal(t, originalJSON, resultJSON)
|
|
})
|
|
|
|
t.Run("should remove precomputed JSON when a hook makes changes", func(t *testing.T) {
|
|
event := model.NewWebSocketEvent(model.WebsocketEventPosted, "", "", "", nil, "")
|
|
event = event.PrecomputeJSON()
|
|
|
|
// Ensure that the event has precomputed JSON because changes aren't included when ToJSON is called again
|
|
originalJSON, _ := event.ToJSON()
|
|
event.Add("data", 1234)
|
|
eventJSON, _ := event.ToJSON()
|
|
require.Equal(t, originalJSON, eventJSON)
|
|
|
|
hookIDs := []string{
|
|
broadcastTest,
|
|
}
|
|
hookArgs := []map[string]any{
|
|
{
|
|
"makes_changes": true,
|
|
},
|
|
}
|
|
|
|
result := hub.runBroadcastHooks(event, webConn, hookIDs, hookArgs)
|
|
|
|
eventJSON, _ = event.ToJSON()
|
|
assert.Equal(t, string(originalJSON), string(eventJSON))
|
|
|
|
resultJSON, _ := result.ToJSON()
|
|
assert.NotEqual(t, originalJSON, resultJSON)
|
|
})
|
|
}
|