[orchestrator] Fix task sorting

[anshulpundir]

Typo in task sorter which causes incorrect sorting order, thus causing problems in the task reaper history cleanup logic.

[codecov]

Codecov Report

Merging #2712 into master will increase coverage by 0.19%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2712      +/-   ##
==========================================
+ Coverage   61.69%   61.88%   +0.19%     
==========================================
  Files         134      134              
  Lines       21771    21771              
==========================================
+ Hits        13431    13473      +42     
+ Misses       6892     6840      -52     
- Partials     1448     1458      +10

[chungers]

We could use an unexported function that takes a Status and returns the appropriate Timestamp. If a common function was used to extract the timestamp for either t[i].Status or t[j].Status then this copy-and-paste bug wouldn't have happened.

Thanks for adding a unit test that was sorely needed, as shown by this bug.

[thaJeztah]

I see this was introduced in d489fa5 (#2332) (merged Jul 27, 2017), so likely has to be cherry-picked into all current release-branches

[thaJeztah]

Looks like this sleep is not needed, because we manually set the timestamp

[thaJeztah]

Timestamp.Seconds can be set directly to the desired value here;

Timestamp: &google_protobuf.Timestamp{Seconds: seconds},

[thaJeztah]

See above; this doesn't look needed if it's initialised with the right value

[thaJeztah]

same here; this can be

timestamp := &google_protobuf.Timestamp{Seconds: expected}

This would even be written as;

sort.Sort(TasksByTimestamp(tasks))
for i, task := range tasks {
	expected := &google_protobuf.Timestamp{Seconds: int64(i + 1)}
	assert.Equal(t, expected, task.Status.Timestamp)
}

Also wondering if we want to compare by the task's ID as well (or instead), e.g.;

assert.Equal(t, "id_"+strconv.Itoa(size-(i+1)), task.ID)

[thaJeztah]

task.Status.AppliedAt = &google_protobuf.Timestamp{Seconds: seconds}

[thaJeztah]

Same as above;

sort.Sort(TasksByTimestamp(tasks))
for i, task := range tasks {
	expected := &google_protobuf.Timestamp{Seconds: int64(i + 1)}
	assert.Equal(t, expected, task.Status.AppliedAt)
}

(possibly assert.Equal(t, "id_"+strconv.Itoa(i), task.ID))

[thaJeztah]

Actually; looks like the Spec isn't needed for this test to work; better to remove it

[anshulpundir]

We could use an unexported function that takes a Status and returns the appropriate Timestamp. If a common function was used to extract the timestamp for either t[i].Status or t[j].Status then this copy-and-paste bug wouldn't have happened.

Done.

[chungers]

LGTM