Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEVPROD-7742 Fix current testifylint lint issues (manual) #8604

Merged
2 changes: 1 addition & 1 deletion .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@ linters:
- govet
- misspell
- unconvert
- unused
- unused
2 changes: 2 additions & 0 deletions model/distro/distro_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,7 @@ func TestGetResolvedPlannerSettings(t *testing.T) {
// Fallback to the SchedulerConfig.ExpectedRuntimeFactor as PlannerSettings.ExpectedRunTimeFactor is equal to 0.
assert.EqualValues(t, 7, resolved0.ExpectedRuntimeFactor)
assert.EqualValues(t, 20, resolved0.GenerateTaskFactor)
//nolint:testifylint // We expect it to be exactly 10.
Copy link
Contributor

@Kimchelly Kimchelly Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, what does testifylint not like about this + the others? It seems like checking that some output value exactly matches an expected hard-coded value would be a normal thing to do in a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These ones are checking float. Testifylint suggests we use a (I didn't know it existed) InEpsilon assertion/require for floats to account for floating point number errors. I figured since our tests are passing, no reason to modify the behavior to check for a range.

assert.EqualValues(t, 10, resolved0.NumDependentsFactor)
assert.EqualValues(t, 40, resolved0.StepbackTaskFactor)

Expand Down Expand Up @@ -475,6 +476,7 @@ func TestGetResolvedPlannerSettings(t *testing.T) {
assert.EqualValues(t, 0, resolved1.MainlineTimeInQueueFactor)
assert.EqualValues(t, 0, resolved1.ExpectedRuntimeFactor)
assert.EqualValues(t, 0, resolved1.GenerateTaskFactor)
//nolint:testifylint // We expect it to be exactly 0.
assert.EqualValues(t, 0, resolved1.NumDependentsFactor)

ps := &PlannerSettings{
Expand Down
3 changes: 3 additions & 0 deletions model/reliability/query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,7 @@ func TestGetTaskStatsOneDocument(t *testing.T) {
docs, err := GetTaskReliabilityScores(filter)
require.NoError(err)
require.Len(docs, 1)
//nolint:testifylint // We expect it to be exactly 0.42.
assert.Equal(docs[0].SuccessRate, float64(.42))
}

Expand All @@ -344,7 +345,9 @@ func TestGetTaskStatsTwoDocuments(t *testing.T) {
docs, err := GetTaskReliabilityScores(filter)
require.NoError(err)
require.Len(docs, 2)
//nolint:testifylint // We expect it to be exactly equal.
assert.Equal(docs[0].SuccessRate, float64(.56))
//nolint:testifylint // We expect it to be exactly equal.
assert.Equal(docs[1].SuccessRate, float64(.42))
}

Expand Down
1 change: 1 addition & 0 deletions model/task/expected_duration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ func TestExpectedDuration(t *testing.T) {

results, err := getExpectedDurationsForWindow("", project, bv, now.Add(-1*time.Hour), now)
assert.NoError(err)
//nolint:testifylint // We expect it to be exactly equal.
assert.EqualValues(25*time.Minute, results[0].ExpectedDuration)
assert.InDelta(9.35*float64(time.Minute), results[0].StdDev, 0.01*float64(time.Minute))
}
3 changes: 3 additions & 0 deletions model/task/task_annotations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ func TestAddIssueToAnnotation(t *testing.T) {
assert.NotNil(t, annotation.Issues[0].Source)
assert.Equal(t, annotations.UIRequester, annotation.Issues[0].Source.Requester)
assert.Equal(t, "annie.black", annotation.Issues[0].Source.Author)
//nolint:testifylint // We expect it to be exactly equal.
assert.Equal(t, float64(91.23), annotation.Issues[0].ConfidenceScore)

assert.NoError(t, AddIssueToAnnotation(ctx, "t1", 0, issue, "not.annie.black"))
Expand All @@ -38,6 +39,7 @@ func TestAddIssueToAnnotation(t *testing.T) {
assert.NotNil(t, annotation)
assert.Len(t, annotation.Issues, 2)
assert.NotNil(t, annotation.Issues[1].Source)
//nolint:testifylint // We expect it to be exactly equal.
assert.Equal(t, float64(91.23), annotation.Issues[0].ConfidenceScore)
assert.Equal(t, "not.annie.black", annotation.Issues[1].Source.Author)
dbTask, err := FindOneId("t1")
Expand Down Expand Up @@ -181,6 +183,7 @@ func TestPatchIssue(t *testing.T) {
assert.Equal(t, annotations.UIRequester, annotation.Issues[0].Source.Requester)
assert.Equal(t, "bynn.lee", annotation.Issues[0].Source.Author)
assert.Equal(t, "EVG-1234", annotation.Issues[0].IssueKey)
//nolint:testifylint // We expect it to be exactly equal.
assert.Equal(t, float64(91.23), annotation.Issues[0].ConfidenceScore)
assert.Len(t, annotation.SuspectedIssues, 1)
assert.NotNil(t, annotation.SuspectedIssues[0].Source)
Expand Down
58 changes: 21 additions & 37 deletions model/task_queue_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -973,15 +973,7 @@ func (s *taskDAGDispatchServiceSuite) TestIsRefreshFindNextTaskThreadSafe() {
}
s.Require().NoError(d.Insert(s.ctx))

items := []TaskQueueItem{}
for i := 0; i < 50; i++ {
items = append(items, TaskQueueItem{
Id: fmt.Sprintf("%d", i),
BuildVariant: "variant_1",
Version: "version_1",
Project: "project_1",
GroupMaxHosts: 0,
})
t := task.Task{
Id: fmt.Sprintf("%d", i),
BuildVariant: "variant_1",
Expand Down Expand Up @@ -1029,15 +1021,7 @@ func (s *taskDAGDispatchServiceSuite) TestIsRefreshFindNextTaskThreadSafe() {

func (s *taskDAGDispatchServiceSuite) TestFindNextTaskThreadSafe() {
s.Require().NoError(db.ClearCollections(task.Collection))
items := []TaskQueueItem{}
for i := 0; i < 100; i++ {
items = append(items, TaskQueueItem{
Id: fmt.Sprintf("%d", i),
BuildVariant: "variant_1",
Version: "version_1",
Project: "project_1",
GroupMaxHosts: 0,
})
t := task.Task{
Id: fmt.Sprintf("%d", i),
BuildVariant: "variant_1",
Expand Down Expand Up @@ -1068,7 +1052,13 @@ func (s *taskDAGDispatchServiceSuite) TestFindNextTaskThreadSafe() {
defer wg.Done()
<-wait
item := service.FindNextTask(s.ctx, spec, utility.ZeroTime)
s.Require().NotNil(item)
s.NotNil(item)
if item == nil {
// We cannot require item to not be nil because the check would happen in
// a goroutine. This could cause the test to behave nondeterministically.
// See https://github.com/Antonboom/testifylint?tab=readme-ov-file#go-require.
return
}
mu.Lock()
dispatchedTasks[item.Id] = true
mu.Unlock()
Expand All @@ -1089,18 +1079,9 @@ func (s *taskDAGDispatchServiceSuite) TestFindNextTaskThreadSafe() {

func (s *taskDAGDispatchServiceSuite) TestFindNextTaskGroupTaskThreadSafe() {
s.Require().NoError(db.ClearCollections(task.Collection))
items := []TaskQueueItem{}
for i := 0; i < 20; i++ {
groupNum := i / 5
id := fmt.Sprintf("%d", i)
items = append(items, TaskQueueItem{
Id: id,
Group: fmt.Sprintf("group_%d", groupNum),
BuildVariant: "variant_1",
Version: "version_1",
Project: "project_1",
GroupMaxHosts: 1,
})
t := task.Task{
Id: id,
TaskGroup: fmt.Sprintf("group_%d", groupNum),
Expand Down Expand Up @@ -1137,7 +1118,13 @@ func (s *taskDAGDispatchServiceSuite) TestFindNextTaskGroupTaskThreadSafe() {
defer wg.Done()
<-wait
item := service.FindNextTask(s.ctx, spec, utility.ZeroTime)
s.Require().NotNil(item)
s.NotNil(item)
if item == nil {
// We cannot require item to not be nil because the check would happen in
// a goroutine. This could cause the test to behave nondeterministically.
// See https://github.com/Antonboom/testifylint?tab=readme-ov-file#go-require.
return
}
mu.Lock()
dispatchedTasks[item.Id] = true
mu.Unlock()
Expand All @@ -1156,18 +1143,9 @@ func (s *taskDAGDispatchServiceSuite) TestFindNextTaskGroupTaskThreadSafe() {
s.Equal(dispatchedCount, numGoroutines)

s.Require().NoError(db.ClearCollections(task.Collection))
items = []TaskQueueItem{}
for i := 0; i < 20; i++ {
groupNum := i / 5
id := fmt.Sprintf("%d", i)
items = append(items, TaskQueueItem{
Id: id,
Group: fmt.Sprintf("group_%d", groupNum),
BuildVariant: "variant_1",
Version: "version_1",
Project: "project_1",
GroupMaxHosts: 1,
})
t := task.Task{
Id: id,
TaskGroup: fmt.Sprintf("group_%d", groupNum),
Expand Down Expand Up @@ -1199,7 +1177,13 @@ func (s *taskDAGDispatchServiceSuite) TestFindNextTaskGroupTaskThreadSafe() {
defer wg.Done()
<-wait
item := service.FindNextTask(s.ctx, spec, utility.ZeroTime)
s.Require().NotNil(item)
s.NotNil(item)
if item == nil {
// We cannot require item to not be nil because the check would happen in
// a goroutine. This could cause the test to behave nondeterministically.
// See https://github.com/Antonboom/testifylint?tab=readme-ov-file#go-require.
return
}
mu.Lock()
dispatchedTasks[item.Id] = true
mu.Unlock()
Expand Down
1 change: 1 addition & 0 deletions model/taskstats/query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,7 @@ func (s *statsQuerySuite) checkTaskStats(stats TaskStats, task, variant, distro
require.Equal(numTestFailed, stats.NumTestFailed)
require.Equal(numSystemFailed, stats.NumSystemFailed)
require.Equal(numSetupFailed, stats.NumSetupFailed)
//nolint:testifylint // We expect these to be exactly equal.
require.Equal(avgDuration, stats.AvgDurationSuccess)
}

Expand Down
1 change: 1 addition & 0 deletions model/taskstats/stats_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ func (s *statsSuite) TestGenerateStats() {
s.Equal(2, doc.NumSystemFailed)
s.Equal(3, doc.NumSetupFailed)
s.Equal(2, doc.NumTimeout)
//nolint:testifylint // We expect it to be exactly 150.0.
s.Equal(float64(150), doc.AvgDurationSuccess)
s.WithinDuration(time.Now(), doc.LastUpdate, time.Minute)

Expand Down
4 changes: 2 additions & 2 deletions rest/data/host_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ func (s *HostConnectorSuite) TestSpawnHost() {
s.Contains(err.Error(), "not been allowed by admins")
},
} {
s.T().Run(tName, func(t *testing.T) {
s.Run(tName, func() {
s.Require().NoError(db.ClearCollections(host.Collection))

options := &restmodel.HostRequestOptions{
Expand All @@ -253,7 +253,7 @@ func (s *HostConnectorSuite) TestSpawnHost() {
InstanceTags: nil,
}

tCase(t, options)
tCase(s.T(), options)
})
}
}
Expand Down
27 changes: 15 additions & 12 deletions rest/model/reliability_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,21 @@ func TestAPITaskReliabilityBuildFromService(t *testing.T) {

assert.Equal(serviceDoc.TaskName, *apiDoc.TaskName)
assert.Equal(serviceDoc.BuildVariant, *apiDoc.BuildVariant)
assert.Equal(serviceDoc.Distro, *apiDoc.Distro)
assert.Equal(serviceDoc.Date.Format("2006-01-02"), *apiDoc.Date)
assert.Equal(serviceDoc.NumSuccess, apiDoc.NumSuccess)
assert.Equal(serviceDoc.NumFailed, apiDoc.NumFailed)
assert.Equal(serviceDoc.NumTotal, apiDoc.NumTotal)
assert.Equal(serviceDoc.NumTimeout, apiDoc.NumTimeout)
assert.Equal(serviceDoc.NumTestFailed, apiDoc.NumTestFailed)
assert.Equal(serviceDoc.NumSystemFailed, apiDoc.NumSystemFailed)
assert.Equal(serviceDoc.NumSetupFailed, apiDoc.NumSetupFailed)
assert.Equal(serviceDoc.SuccessRate, apiDoc.SuccessRate)
assert.Equal(serviceDoc.AvgDurationSuccess, apiDoc.AvgDurationSuccess)
assert.Equal(serviceDoc.SuccessRate, 8.0)
assert.Equal(*apiDoc.Distro, serviceDoc.Distro)
assert.Equal(*apiDoc.Date, serviceDoc.Date.Format("2006-01-02"))
assert.Equal(apiDoc.NumSuccess, serviceDoc.NumSuccess)
assert.Equal(apiDoc.NumFailed, serviceDoc.NumFailed)
assert.Equal(apiDoc.NumTotal, serviceDoc.NumTotal)
assert.Equal(apiDoc.NumTimeout, serviceDoc.NumTimeout)
assert.Equal(apiDoc.NumTestFailed, serviceDoc.NumTestFailed)
assert.Equal(apiDoc.NumSystemFailed, serviceDoc.NumSystemFailed)
assert.Equal(apiDoc.NumSetupFailed, serviceDoc.NumSetupFailed)
//nolint:testifylint // We expect it to be exactly equal.
assert.Equal(apiDoc.SuccessRate, serviceDoc.SuccessRate)
//nolint:testifylint // We expect it to be exactly equal.
assert.Equal(apiDoc.AvgDurationSuccess, serviceDoc.AvgDurationSuccess)
//nolint:testifylint // We expect it to be exactly 0.0.
assert.Equal(8.0, serviceDoc.SuccessRate)
}

func TestAPITaskReliabilityStartAtKey(t *testing.T) {
Expand Down
1 change: 1 addition & 0 deletions rest/model/task_stats_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ func TestAPITaskStatsBuildFromService(t *testing.T) {
assert.Equal(serviceDoc.NumTestFailed, apiDoc.NumTestFailed)
assert.Equal(serviceDoc.NumSystemFailed, apiDoc.NumSystemFailed)
assert.Equal(serviceDoc.NumSetupFailed, apiDoc.NumSetupFailed)
//nolint:testifylint // We expect it to be exactly equal.
assert.Equal(serviceDoc.AvgDurationSuccess, apiDoc.AvgDurationSuccess)
}

Expand Down
9 changes: 7 additions & 2 deletions rest/route/annotations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,7 @@ func TestAnnotationByTaskGetHandlerRun(t *testing.T) {
assert.Equal(t, "task-1", utility.FromStringPtr(apiAnnotations[0].TaskId))
assert.Equal(t, "task-1-note_1", utility.FromStringPtr(apiAnnotations[0].Note.Message))
require.Len(t, apiAnnotations[0].Issues, 1)
//nolint:testifylint // We expect it to be exactly equal.
assert.Equal(t, float64(12.34), utility.FromFloat64Ptr(apiAnnotations[0].Issues[0].ConfidenceScore))

// get the latest execution : 0
Expand Down Expand Up @@ -681,8 +682,10 @@ func TestAnnotationByTaskPutHandlerRun(t *testing.T) {
assert.Equal(t, "test_annotation_user", annotation.Note.Source.Author)
assert.Equal(t, "api", annotation.Note.Source.Requester)
assert.Equal(t, "api", annotation.Issues[0].Source.Requester)
assert.Equal(t, 2, len(annotation.Issues))
assert.Len(t, annotation.Issues, 2)
//nolint:testifylint // We expect it to be exactly equal.
assert.Equal(t, float64(12.34), annotation.Issues[0].ConfidenceScore)
//nolint:testifylint // We expect it to be exactly equal.
assert.Equal(t, float64(56.78), annotation.Issues[1].ConfidenceScore)

//test update
Expand Down Expand Up @@ -714,8 +717,10 @@ func TestAnnotationByTaskPutHandlerRun(t *testing.T) {
// suspected issues and issues don't get updated when not defined
require.Nil(t, annotation.SuspectedIssues)
assert.Equal(t, "some key 0", annotation.Issues[0].IssueKey)
assert.Equal(t, 2, len(annotation.Issues))
assert.Len(t, annotation.Issues, 2)
//nolint:testifylint // We expect it to be exactly equal.
assert.Equal(t, float64(87.65), annotation.Issues[0].ConfidenceScore)
//nolint:testifylint // We expect it to be exactly equal.
assert.Equal(t, float64(43.21), annotation.Issues[1].ConfidenceScore)

//test that it can update old executions
Expand Down
12 changes: 7 additions & 5 deletions rest/route/distro_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -708,8 +708,9 @@ func (s *DistroPatchByIDSuite) TestRunProviderSettingsList() {
doc = apiDistro.ProviderSettingsList[0]
mappedDoc, ok := doc.Lookup("mount_points").MutableArray().Lookup(0).MutableDocumentOK()
s.True(ok)
s.Equal(mappedDoc.Lookup("device_name").StringValue(), "/dev/xvdb")
s.Equal(mappedDoc.Lookup("virtual_name").StringValue(), "ephemeral0")
s.Equal("/dev/xvdb", mappedDoc.Lookup("device_name").StringValue())
s.Equal("ephemeral0", mappedDoc.Lookup("virtual_name").StringValue())
//nolint:testifylint // We expect it to be exactly 0.15.
s.Equal(doc.Lookup("bid_price").Double(), 0.15)
s.Equal(doc.Lookup("instance_type").StringValue(), "m3.large")
s.Equal(doc.Lookup("key_name").StringValue(), "mci")
Expand Down Expand Up @@ -1229,9 +1230,10 @@ func (s *DistroPatchByIDSuite) TestValidFindAndReplaceFullDocument() {
doc := apiDistro.ProviderSettingsList[0]

mountPoint := doc.Lookup("mount_points").MutableArray().Lookup(0).MutableDocument()
s.Equal(mountPoint.Lookup("device_name").StringValue(), "~/dev/xvdb")
s.Equal(mountPoint.Lookup("virtual_name").StringValue(), "~ephemeral0")
s.Equal(doc.Lookup("ami").StringValue(), "~ami-2814683f")
s.Equal("~/dev/xvdb", mountPoint.Lookup("device_name").StringValue())
s.Equal("~ephemeral0", mountPoint.Lookup("virtual_name").StringValue())
s.Equal("~ami-2814683f", doc.Lookup("ami").StringValue())
//nolint:testifylint // We expect it to be exactly 0.10.
s.Equal(doc.Lookup("bid_price").Double(), 0.10)
s.Equal(doc.Lookup("instance_type").StringValue(), "~m3.large")

Expand Down
4 changes: 3 additions & 1 deletion rest/route/reliability_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,8 @@ func TestReliabilityParse(t *testing.T) {
err = handler.parseTaskReliabilityFilter(values)
require.NoError(t, err)
require.Equal(t, values["tasks"], handler.filter.Tasks)
require.Equal(t, handler.filter.Sort, taskstats.SortLatestFirst)
require.Equal(t, taskstats.SortLatestFirst, handler.filter.Sort)
//nolint:testifylint // We expect the float to be exactly equal.
require.Equal(t, handler.filter.Significance, reliability.DefaultSignificance)

require.Equal(t, handler.filter.Requesters, []string{"gitter_request"})
Expand Down Expand Up @@ -430,6 +431,7 @@ func TestReliabilityParse(t *testing.T) {
require.Equal(t, reliability.GroupByDistro, handler.filter.GroupBy) // default value
require.Equal(t, reliability.SortLatestFirst, handler.filter.Sort) // default value
require.Equal(t, reliability.MaxQueryLimit, handler.filter.Limit) // default value
//nolint:testifylint // We expect the float to be exactly 0.1.
require.Equal(t, 0.1, handler.filter.Significance)
},
"Some Values": func(ctx context.Context, t *testing.T, handler taskReliabilityHandler) {
Expand Down
2 changes: 1 addition & 1 deletion units/host_drawdown_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ func TestHostDrawdown(t *testing.T) {
NewCapTarget: 0,
}
num, hosts := numHostsDecommissionedForDrawdown(ctx, t, env, drawdownInfo)
assert.Zero(t, 0, num, "should not draw down host that was recently running task group")
assert.Zero(t, num, "should not draw down host that was recently running task group")
assert.Empty(t, hosts)
},
"DecommissionsIdleMultiHostTaskGroupHost": func(ctx context.Context, t *testing.T, env *mock.Environment, d distro.Distro) {
Expand Down
Loading