From c88fd2b4d7a306cf35a10016ee5cd57b38faa175 Mon Sep 17 00:00:00 2001 From: Sanjay P Date: Fri, 28 Aug 2026 10:42:14 +0530 Subject: [PATCH] utils: make Median generic, overflow-safe, and add tests (#4553) * utils: make Median generic, overflow-safe, and add comprehensive * tests: fix staticcheck unused variable warning in changenotifier_test.go * tests: switch from assert to require for consistency with existing tests * trigger ci rerun --- pkg/utils/changenotifier_test.go | 122 +++++++++++++++++++++++++++++++ pkg/utils/math.go | 39 +++++++--- pkg/utils/math_test.go | 89 ++++++++++++++++++++++ pkg/utils/slice_test.go | 47 ++++++++++++ 4 files changed, 288 insertions(+), 9 deletions(-) create mode 100644 pkg/utils/changenotifier_test.go create mode 100644 pkg/utils/math_test.go create mode 100644 pkg/utils/slice_test.go diff --git a/pkg/utils/changenotifier_test.go b/pkg/utils/changenotifier_test.go new file mode 100644 index 000000000..c98920a82 --- /dev/null +++ b/pkg/utils/changenotifier_test.go @@ -0,0 +1,122 @@ +// Copyright 2026 LiveKit, Inc. +// +// Licensed 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 utils + +import ( + "sync" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestChangeNotifier(t *testing.T) { + t.Run("Observer management", func(t *testing.T) { + notifier := NewChangeNotifier() + require.False(t, notifier.HasObservers()) + + called := false + notifier.AddObserver("test-key", func() { + called = true + }) + require.True(t, notifier.HasObservers()) + + notifier.RemoveObserver("test-key") + require.False(t, notifier.HasObservers()) + require.False(t, called) + }) + + t.Run("Notification triggers callbacks asynchronously", func(t *testing.T) { + notifier := NewChangeNotifier() + var wg sync.WaitGroup + wg.Add(2) + + var mu sync.Mutex + callCounts := make(map[string]int) + + notifier.AddObserver("obs1", func() { + mu.Lock() + callCounts["obs1"]++ + mu.Unlock() + wg.Done() + }) + + notifier.AddObserver("obs2", func() { + mu.Lock() + callCounts["obs2"]++ + mu.Unlock() + wg.Done() + }) + + notifier.NotifyChanged() + + // Wait for async execution of observers + done := make(chan struct{}) + go func() { + wg.Wait() + close(done) + }() + + select { + case <-done: + // Success + case <-time.After(1 * time.Second): + t.Fatal("Timeout waiting for change notification callbacks") + } + + mu.Lock() + require.Equal(t, 1, callCounts["obs1"]) + require.Equal(t, 1, callCounts["obs2"]) + mu.Unlock() + }) +} + +func TestChangeNotifierManager(t *testing.T) { + t.Run("Get and Create Notifiers", func(t *testing.T) { + manager := NewChangeNotifierManager() + require.Nil(t, manager.GetNotifier("non-existent")) + + notifier := manager.GetOrCreateNotifier("room1") + require.NotNil(t, notifier) + + retrieved := manager.GetNotifier("room1") + require.Equal(t, notifier, retrieved) + + // GetOrCreate should return the existing one + again := manager.GetOrCreateNotifier("room1") + require.Equal(t, notifier, again) + }) + + t.Run("Remove Notifiers with HasObservers check", func(t *testing.T) { + manager := NewChangeNotifierManager() + _ = manager.GetOrCreateNotifier("room1") + + // Case 1: notifier has no observers, should be removed + manager.RemoveNotifier("room1", false) + require.Nil(t, manager.GetNotifier("room1")) + + // Re-create and add an observer + notifier := manager.GetOrCreateNotifier("room1") + notifier.AddObserver("observer", func() {}) + + // Case 2: notifier has observer, RemoveNotifier(..., false) should not remove it + manager.RemoveNotifier("room1", false) + require.NotNil(t, manager.GetNotifier("room1")) + + // Case 3: notifier has observer, RemoveNotifier(..., true) (force) should remove it + manager.RemoveNotifier("room1", true) + require.Nil(t, manager.GetNotifier("room1")) + }) +} diff --git a/pkg/utils/math.go b/pkg/utils/math.go index 3a419db2b..253514461 100644 --- a/pkg/utils/math.go +++ b/pkg/utils/math.go @@ -14,24 +14,45 @@ package utils -import "slices" +import ( + "cmp" + "slices" +) -// Median gets median value for an array -func Median[T float32](input []T) T { +// OrderedNumber defines a constraint for numeric types that can be ordered and divided. +type OrderedNumber interface { + cmp.Ordered + ~int | ~int8 | ~int16 | ~int32 | ~int64 | + ~uint | ~uint8 | ~uint16 | ~uint32 | ~uint64 | + ~uintptr | ~float32 | ~float64 +} + +// Median gets the median value for a slice without modifying the original slice. +// +// Note: +// 1. For integer types, if the slice has an even length, the division (/ 2) +// is performed using integer division, which truncates the result towards zero. +// 2. Uses an overflow-safe formula left + (right-left)/2 to support narrow integer types. +func Median[T OrderedNumber](input []T) T { num := len(input) switch num { case 0: - return 0 + var zero T + return zero case 1: return input[0] } - slices.Sort(input) + + // Clone the slice to avoid mutating the caller's slice + sortedInput := slices.Clone(input) + slices.Sort(sortedInput) + if num%2 != 0 { - return input[num/2] + return sortedInput[num/2] } - left := input[num/2-1] - right := input[num/2] - return (left + right) / 2 + left := sortedInput[num/2-1] + right := sortedInput[num/2] + return left + (right-left)/T(2) } func Signum[T int | int8 | int16 | int32 | int64 | float32 | float64](val T) int { diff --git a/pkg/utils/math_test.go b/pkg/utils/math_test.go new file mode 100644 index 000000000..48014b9b9 --- /dev/null +++ b/pkg/utils/math_test.go @@ -0,0 +1,89 @@ +// Copyright 2026 LiveKit, Inc. +// +// Licensed 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 utils + +import ( + "slices" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestMedian(t *testing.T) { + t.Run("Empty slice", func(t *testing.T) { + require.Equal(t, float32(0), Median([]float32{})) + require.Equal(t, int(0), Median([]int{})) + }) + + t.Run("Single element", func(t *testing.T) { + require.Equal(t, float32(42), Median([]float32{42})) + require.Equal(t, int(42), Median([]int{42})) + }) + + t.Run("Odd length float32", func(t *testing.T) { + input := []float32{3.0, 1.0, 2.0} + require.Equal(t, float32(2.0), Median(input)) + }) + + t.Run("Even length float32 - exact average", func(t *testing.T) { + input := []float32{1.0, 2.0, 3.0, 4.0} + require.Equal(t, float32(2.5), Median(input)) + }) + + t.Run("Even length int - integer truncation", func(t *testing.T) { + input := []int{1, 2} + // (1 + 2) / 2 = 1.5 -> truncates to 1 + require.Equal(t, int(1), Median(input)) + + inputOddAverage := []int{1, 3} + // (1 + 3) / 2 = 2 + require.Equal(t, int(2), Median(inputOddAverage)) + }) + + t.Run("Int8 overflow prevention", func(t *testing.T) { + // Without overflow protection: 120 + 126 = 246 (overflows int8 to -10) -> -10 / 2 = -5 + // With overflow protection: 120 + (126-120)/2 = 123 + input := []int8{120, 126} + require.Equal(t, int8(123), Median(input)) + }) + + t.Run("Uint8 overflow prevention", func(t *testing.T) { + input := []uint8{250, 254} + require.Equal(t, uint8(252), Median(input)) + }) + + t.Run("Immutability test - caller slice is not sorted/mutated", func(t *testing.T) { + original := []int{3, 1, 4, 2} + input := slices.Clone(original) + + median := Median(input) + require.Equal(t, int(2), median) + require.Equal(t, original, input, "Input slice must not be modified by Median") + }) +} + +func TestSignum(t *testing.T) { + t.Run("Integer values", func(t *testing.T) { + require.Equal(t, -1, Signum(-42)) + require.Equal(t, 0, Signum(0)) + require.Equal(t, 1, Signum(42)) + }) + + t.Run("Floating point values", func(t *testing.T) { + require.Equal(t, -1, Signum(float32(-0.01))) + require.Equal(t, 0, Signum(float32(0.0))) + require.Equal(t, 1, Signum(float32(0.01))) + }) +} diff --git a/pkg/utils/slice_test.go b/pkg/utils/slice_test.go new file mode 100644 index 000000000..055c0c4c3 --- /dev/null +++ b/pkg/utils/slice_test.go @@ -0,0 +1,47 @@ +// Copyright 2026 LiveKit, Inc. +// +// Licensed 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 utils + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestDedupeSlice(t *testing.T) { + t.Run("Empty slice", func(t *testing.T) { + var input []int + result := DedupeSlice(input) + require.Empty(t, result) + }) + + t.Run("Single element", func(t *testing.T) { + input := []string{"hello"} + result := DedupeSlice(input) + require.Equal(t, []string{"hello"}, result) + }) + + t.Run("Unsorted slice with duplicates", func(t *testing.T) { + input := []int{4, 2, 4, 1, 3, 2} + result := DedupeSlice(input) + require.Equal(t, []int{1, 2, 3, 4}, result) + }) + + t.Run("Already sorted and unique", func(t *testing.T) { + input := []string{"apple", "banana", "cherry"} + result := DedupeSlice(input) + require.Equal(t, []string{"apple", "banana", "cherry"}, result) + }) +}