Fix: test time #586
Fix: test time #586
Conversation
| @@ -53,8 +53,6 @@ func TestPersistentRunner(t *testing.T) { | |||
|
|
|||
| run2 := f.mockDB.runs[test2.ID.String()] | |||
| require.NotNil(t, run2) | |||
|
|
|||
| assert.True(t, run1.CompletedAt.UnixNano() > run2.CompletedAt.UnixNano(), "test1 did not complete after test2") | |||
There was a problem hiding this comment.
Does this test still make sense? I feel that having a runner test mocking everything doesn't provide great value right now that we have integration tests.
There was a problem hiding this comment.
Yes, this test that the runner is actually async, that was why the comparison was by dates. This needs to be kept. Maybe we could add a runCompletedAt field or similar, so we keep track of all the timestmaps of the process
mathnogueira
force-pushed
the
fix/test-time
branch
from
374f3bc
to
5bad5dc
Compare
| @@ -53,8 +53,6 @@ func TestPersistentRunner(t *testing.T) { | |||
|
|
|||
| run2 := f.mockDB.runs[test2.ID.String()] | |||
| require.NotNil(t, run2) | |||
|
|
|||
| assert.True(t, run1.CompletedAt.UnixNano() > run2.CompletedAt.UnixNano(), "test1 did not complete after test2") | |||
There was a problem hiding this comment.
Yes, this test that the runner is actually async, that was why the comparison was by dates. This needs to be kept. Maybe we could add a runCompletedAt field or similar, so we keep track of all the timestmaps of the process
server/executor/runner.go
Outdated
| @@ -70,6 +70,7 @@ func (r persistentRunner) Start(workers int) { | |||
| fmt.Println("persistentRunner exit goroutine") | |||
| return | |||
| case job := <-r.executeQueue: | |||
| job.run.StartedAt = time.Now() | |||
There was a problem hiding this comment.
I would put this either at the processExecQueue func, or remove the field, since it's almost the same value as ServiceTriggeredAt
server/executor/trace_poller.go
Outdated
| @@ -201,7 +202,7 @@ func (tp tracePoller) donePollingTraces(job tracePollReq, trace traces.Trace) bo | |||
| func (tp tracePoller) handleTraceDBError(job tracePollReq, err error) { | |||
| run := job.run | |||
| if errors.Is(err, tracedb.ErrTraceNotFound) { | |||
| if time.Since(run.CompletedAt) < tp.maxWaitTimeForTrace { | |||
| if time.Since(run.CreatedAt) < tp.maxWaitTimeForTrace { | |||
There was a problem hiding this comment.
This feels weird. This time now includes the time elapsed since the trigger started, so we need to either change the config name, or change the checked field. I'd incline for the latter, so we can have different timeouts for execution and trace polling
server/executor/runner.go
Outdated
| SpanID: r.idGen.SpanID(), | ||
| State: model.RunStateCreated, | ||
| CreatedAt: time.Now(), | ||
| ServiceTriggeredAt: time.Time{}, |
There was a problem hiding this comment.
you don't need to do this. If you just leave them empty, they will be zero valued
This PR fixes
CompletedAtvalue. Now it is populated at the end of the run or when it fails.I also added a few more time variables so we can know know much time each step took.
Changes
Fixes
Checklist