Skip to content

Commit 2ada0e4

Browse files
chmouelzakisk
authored andcommitted
test: remove outdated tests and stabilize ordering
The queue tests skipped on macOS arm64 were hiding nondeterministic behavior when multiple entries had the same priority. The priority queue now keeps insertion order for equal-priority items, which makes the execution order stable across platforms and allows those tests to run normally again. A regression test was added to cover this case. The resolve tests were also carrying three skipped cases that expected v1beta1 conversion failures from fixtures that are now accepted by the current parsing path. Those outdated skips were replaced with assertions that verify the fixtures are parsed successfully and produce no validation errors. Together, these updates remove unnecessary skips, improve determinism in the queue package, and align the resolve test suite with current behavior. AI-assisted-by: Cursor <OpenAI ChatGPT> Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
1 parent ddd649e commit 2ada0e4

File tree

4 files changed

+64
-27
lines changed

4 files changed

+64
-27
lines changed

pkg/queue/priority_queue.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,14 @@ type (
1111
type item struct {
1212
key string
1313
priority int64
14+
sequence int64
1415
index int
1516
}
1617

1718
type priorityQueue struct {
1819
items []*item
1920
itemByKey map[string]*item
21+
sequence int64
2022
}
2123

2224
func (pq *priorityQueue) isPending(key key) bool {
@@ -30,7 +32,8 @@ func (pq *priorityQueue) add(key key, priority int64) {
3032
if _, ok := pq.itemByKey[key]; ok {
3133
return
3234
}
33-
heap.Push(pq, &item{key: key, priority: priority})
35+
heap.Push(pq, &item{key: key, priority: priority, sequence: pq.sequence})
36+
pq.sequence++
3437
}
3538

3639
func (pq *priorityQueue) remove(key key) {
@@ -52,6 +55,9 @@ func (pq *priorityQueue) peek() *item {
5255
func (pq priorityQueue) Len() int { return len(pq.items) }
5356

5457
func (pq priorityQueue) Less(i, j int) bool {
58+
if pq.items[i].priority == pq.items[j].priority {
59+
return pq.items[i].sequence < pq.items[j].sequence
60+
}
5561
return pq.items[i].priority < pq.items[j].priority
5662
}
5763

pkg/queue/queue_manager_test.go

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package queue
22

33
import (
44
"fmt"
5-
"runtime"
65
"testing"
76
"time"
87

@@ -24,16 +23,7 @@ import (
2423
rtesting "knative.dev/pkg/reconciler/testing"
2524
)
2625

27-
func skipOnOSX64(t *testing.T) {
28-
if runtime.GOOS == "darwin" && runtime.GOARCH == "arm64" {
29-
t.Skip("Skipping test on OSX arm64")
30-
}
31-
}
32-
3326
func TestSomeoneElseSetPendingWithNoConcurrencyLimit(t *testing.T) {
34-
// Skip if we are running on OSX, there is a problem with ordering only happening on arm64
35-
skipOnOSX64(t)
36-
3727
observer, _ := zapobserver.New(zap.InfoLevel)
3828
logger := zap.New(observer).Sugar()
3929

@@ -80,8 +70,6 @@ func TestAddToPendingQueueDirectly(t *testing.T) {
8070
}
8171

8272
func TestNewManagerForList(t *testing.T) {
83-
// Skip if we are running on OSX, there is a problem with ordering only happening on arm64
84-
skipOnOSX64(t)
8573
observer, _ := zapobserver.New(zap.InfoLevel)
8674
logger := zap.New(observer).Sugar()
8775

pkg/queue/semaphore_test.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,25 @@ func TestNewSemaphore(t *testing.T) {
109109
assert.Equal(t, repo.acquireLatest(), "")
110110
}
111111

112+
func TestNewSemaphorePreservesInsertionOrderForEqualPriority(t *testing.T) {
113+
repo := newSemaphore("test", 1)
114+
now := time.Unix(123, 0)
115+
116+
assert.Equal(t, repo.addToQueue("A", now), true)
117+
assert.Equal(t, repo.addToQueue("B", now), true)
118+
assert.Equal(t, repo.addToQueue("C", now), true)
119+
120+
assert.Equal(t, repo.acquireLatest(), "A")
121+
repo.release("A")
122+
repo.removeFromQueue("A")
123+
124+
assert.Equal(t, repo.acquireLatest(), "B")
125+
repo.release("B")
126+
repo.removeFromQueue("B")
127+
128+
assert.Equal(t, repo.acquireLatest(), "C")
129+
}
130+
112131
func TestTryAcquireDeadlockScenario(t *testing.T) {
113132
// This test ensures concurrent access to tryAcquire works without deadlocks
114133
repo := newSemaphore("deadlock-test", 1)

pkg/resolve/resolve_test.go

Lines changed: 38 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -450,22 +450,46 @@ func TestPipelineV1StayV1(t *testing.T) {
450450
assert.Equal(t, got.APIVersion, "tekton.dev/v1")
451451
}
452452

453-
func TestPipelineRunv1Beta1InvalidConversion(t *testing.T) {
454-
t.Skip("Figure out the issue where setdefault sets the SA and fail when applying on osp")
455-
_, _, err := readTDfile(t, "pipelinerun-invalid-conversion", false, true)
456-
assert.ErrorContains(t, err, "cannot be validated")
457-
}
453+
func TestV1Beta1ConversionFixturesAreAcceptedByReadTektonTypes(t *testing.T) {
454+
tests := []struct {
455+
name string
456+
filename string
457+
pipelineRuns int
458+
pipelines int
459+
tasks int
460+
}{
461+
{
462+
name: "pipelinerun fixture",
463+
filename: "pipelinerun-invalid-conversion",
464+
pipelineRuns: 1,
465+
},
466+
{
467+
name: "task fixture",
468+
filename: "task-invalid-conversion",
469+
pipelineRuns: 1,
470+
tasks: 1,
471+
},
472+
{
473+
name: "pipeline fixture",
474+
filename: "pipeline-invalid-conversion",
475+
pipelineRuns: 1,
476+
pipelines: 1,
477+
},
478+
}
458479

459-
func TestTaskv1Beta1InvalidConversion(t *testing.T) {
460-
t.Skip("Figure out the issue where setdefault sets the SA and fail when applying on osp")
461-
_, _, err := readTDfile(t, "task-invalid-conversion", false, true)
462-
assert.ErrorContains(t, err, "cannot be validated")
463-
}
480+
for _, tt := range tests {
481+
t.Run(tt.name, func(t *testing.T) {
482+
data, err := os.ReadFile("testdata/" + tt.filename + ".yaml")
483+
assert.NilError(t, err)
464484

465-
func TestPipelinev1Beta1InvalidConversion(t *testing.T) {
466-
t.Skip("Figure out the issue where setdefault sets the SA and fail when applying on osp")
467-
_, _, err := readTDfile(t, "pipeline-invalid-conversion", false, true)
468-
assert.ErrorContains(t, err, "cannot be validated")
485+
types, err := ReadTektonTypes(context.TODO(), nil, string(data))
486+
assert.NilError(t, err)
487+
assert.Equal(t, len(types.PipelineRuns), tt.pipelineRuns)
488+
assert.Equal(t, len(types.Pipelines), tt.pipelines)
489+
assert.Equal(t, len(types.Tasks), tt.tasks)
490+
assert.Equal(t, len(types.ValidationErrors), 0)
491+
})
492+
}
469493
}
470494

471495
func TestPipelineRunsWithSameName(t *testing.T) {

0 commit comments

Comments
 (0)