-
Notifications
You must be signed in to change notification settings - Fork 71
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
Add TestSpec events #2280
Add TestSpec events #2280
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job @danielbdias !
server/model/test_run_event.go
Outdated
} | ||
|
||
func NewTestRunEventWithArgs(stage TestRunEventStage, eventType TestRunEventType, testID id.ID, runID int, titleAndDescriptionArgs map[string]string) TestRunEvent { | ||
eventTypeTable, found := eventTable[stage] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this for events that have extra information attached to them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, by following @mathnogueira we have a less complex way to do that, for instance here: https://github.com/kubeshop/tracetest/pull/2280/files#diff-cc7adc54283b5ac608363703f8b26f6b0c084cf35c520e7d06ed20da9dd0fadeR206
server/executor/assertion_runner.go
Outdated
@@ -103,19 +105,26 @@ func (e *defaultAssertionRunner) startWorker() { | |||
|
|||
func (e *defaultAssertionRunner) runAssertionsAndUpdateResult(ctx context.Context, request AssertionRequest) (model.Run, error) { | |||
log.Printf("[AssertionRunner] Test %s Run %d: Starting\n", request.Test.ID, request.Run.ID) | |||
|
|||
e.eventEmitter.Emit(ctx, model.NewTestRunEvent(model.StageTest, model.TestEventType_TestSpecsRunStart, request.Test.ID, request.Run.ID)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some pieces of information missing here. An event should also contain a title and a description. Maybe, this is not the best way of emitting an event.
Instead of just declaring the types as constants in a file, I think it would be nice to have a package to centralize the creation of those events, so instead of:
e.eventEmitter.Emit(ctx, model.NewTestRunEvent(model.StageTest, model.TestEventType_TestSpecsRunStart, request.Test.ID, request.Run.ID))
We could have:
e.eventEmitter.Emit(ctx, events.TriggerCreated(request.Test.ID, request.Run.ID)
Each constructor would have a different set of parameters (testID and RunID are required in all of them) to match the description and title we want to provide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! Changing it!
server/model/test_run_event.go
Outdated
var ( | ||
eventTable map[TestRunEventStage]map[TestRunEventType]TestRunEvent = map[TestRunEventStage]map[TestRunEventType]TestRunEvent{ | ||
StageTrigger: map[TestRunEventType]TestRunEvent{ | ||
TriggerEventType_CreatedInfo: TestRunEvent{Stage: StageTrigger, Type: TriggerEventType_CreatedInfo, Title: "Trigger Run has been created", Description: "Trigger Run has been created"}, | ||
TriggerEventType_ResolveError: TestRunEvent{Stage: StageTrigger, Type: TriggerEventType_ResolveError, Title: "Resolving trigger details failed", Description: "Resolving trigger details failed"}, | ||
TriggerEventType_ResolveSuccess: TestRunEvent{Stage: StageTrigger, Type: TriggerEventType_ResolveSuccess, Title: "Successful resolving of trigger details", Description: "Successful resolving of trigger details"}, | ||
TriggerEventType_ResolveStart: TestRunEvent{Stage: StageTrigger, Type: TriggerEventType_ResolveStart, Title: "Resolving trigger details based on environment variables", Description: "Resolving trigger details based on environment variables"}, | ||
TriggerEventType_ExecutionStart: TestRunEvent{Stage: StageTrigger, Type: TriggerEventType_ExecutionStart, Title: "Initial trigger execution", Description: "Initial trigger execution"}, | ||
TriggerEventType_ExecutionSuccess: TestRunEvent{Stage: StageTrigger, Type: TriggerEventType_ExecutionSuccess, Title: "Successful trigger execution", Description: "Successful trigger execution"}, | ||
TriggerEventType_HTTPUnreachableHostError: TestRunEvent{Stage: StageTrigger, Type: TriggerEventType_HTTPUnreachableHostError, Title: "Tracetest could not reach the defined host in the trigger", Description: "Tracetest could not reach the defined host in the trigger"}, | ||
TriggerEventType_DockerComposeHostMismatchError: TestRunEvent{Stage: StageTrigger, Type: TriggerEventType_DockerComposeHostMismatchError, Title: "Tracetest is running inside a Docker container", Description: "We identified Tracetest is running inside a docker compose container, so if you are trying to access your local host machine please use the host.docker.internal hostname. For more information, see https://docs.docker.com/docker-for-mac/networking/#use-cases-and-workarounds"}, | ||
TriggerEventType_gRPCUnreachableHostError: TestRunEvent{Stage: StageTrigger, Type: TriggerEventType_gRPCUnreachableHostError, Title: "Tracetest could not reach the defined host in the trigger", Description: "Tracetest could not reach the defined host in the trigger"}, | ||
}, | ||
StageTrace: map[TestRunEventType]TestRunEvent{ | ||
TraceEventType_FetchingStart: TestRunEvent{Stage: StageTrace, Type: TraceEventType_FetchingStart, Title: "Starting the trace fetching process", Description: "Starting the trace fetching process"}, | ||
TraceEventType_QueuedInfo: TestRunEvent{Stage: StageTrace, Type: TraceEventType_QueuedInfo, Title: "Trace Run has been queued to start the fetching process", Description: "Trace Run has been queued to start the fetching process"}, | ||
TraceEventType_DataStoreConnectionInfo: TestRunEvent{Stage: StageTrace, Type: TraceEventType_DataStoreConnectionInfo, Title: "A Data store connection request has been executed,test connection result information", Description: "A Data store connection request has been executed,test connection result information"}, | ||
TraceEventType_PollingStart: TestRunEvent{Stage: StageTrace, Type: TraceEventType_PollingStart, Title: "Starting the trace polling process", Description: "Starting the trace polling process"}, | ||
TraceEventType_PollingIterationInfo: TestRunEvent{Stage: StageTrace, Type: TraceEventType_PollingIterationInfo, Title: "A polling iteration has been executed", Description: "A polling iteration has been executed, {{NUMBER_OF_SPANS}} spans - iteration {{ITERATION_NUMBER}} - reason of next iteration: {{ITERATION_REASON}}"}, | ||
TraceEventType_PollingSuccess: TestRunEvent{Stage: StageTrace, Type: TraceEventType_PollingSuccess, Title: "The polling strategy has succeeded in fetching the trace from the Data Store", Description: "The polling strategy has succeeded in fetching the trace from the Data Store"}, | ||
TraceEventType_PollingError: TestRunEvent{Stage: StageTrace, Type: TraceEventType_PollingError, Title: "The polling strategy has failed to fetch the trace", Description: "The polling strategy has failed to fetch the trace"}, | ||
TraceEventType_FetchingSuccess: TestRunEvent{Stage: StageTrace, Type: TraceEventType_FetchingSuccess, Title: "The trace was successfully processed by the backend", Description: "The trace was successfully processed by the backend"}, | ||
TraceEventType_FetchingError: TestRunEvent{Stage: StageTrace, Type: TraceEventType_FetchingError, Title: "The trace was not able to be fetched", Description: "The trace was not able to be fetched"}, | ||
TraceEventType_StoppedInfo: TestRunEvent{Stage: StageTrace, Type: TraceEventType_StoppedInfo, Title: "The test run was stopped during its execution", Description: "The test run was stopped during its execution"}, | ||
}, | ||
StageTest: map[TestRunEventType]TestRunEvent{ | ||
TestEventType_OutputGenerationWarning: TestRunEvent{Stage: StageTest, Type: TestEventType_OutputGenerationWarning, Title: "Output {{OUTPUT_NAME}} not be generated", Description: "The value for output {{OUTPUT_NAME}} could not be generated"}, | ||
TestEventType_ResolveStart: TestRunEvent{Stage: StageTest, Type: TestEventType_ResolveStart, Title: "Resolving test specs details start", Description: "Resolving test specs details start"}, | ||
TestEventType_ResolveSuccess: TestRunEvent{Stage: StageTest, Type: TestEventType_ResolveSuccess, Title: "Resolving test specs details success", Description: "Resolving test specs details success"}, | ||
TestEventType_ResolveError: TestRunEvent{Stage: StageTest, Type: TestEventType_ResolveError, Title: "An error ocurred while parsing the test specs", Description: "An error ocurred while parsing the test specs"}, | ||
TestEventType_TestSpecsRunSuccess: TestRunEvent{Stage: StageTest, Type: TestEventType_TestSpecsRunSuccess, Title: "Test Specs were successfully executed", Description: "Test Specs were successfully executed"}, | ||
TestEventType_TestSpecsRunError: TestRunEvent{Stage: StageTest, Type: TestEventType_TestSpecsRunError, Title: "Test specs execution error", Description: "Test specs execution error"}, | ||
TestEventType_TestSpecsRunStart: TestRunEvent{Stage: StageTest, Type: TestEventType_TestSpecsRunStart, Title: "Test specs execution start", Description: "Test specs execution start"}, | ||
}, | ||
} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't go with this approach because we might want to throw some extra information in the description of the event.
Example: Trigger Run has been created
could become
Endpoint "http://localhost:8080/api/users" was triggered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. By updating to have one constructor per event, we can do this more explicitly in our code.
1ed559b
to
d212a77
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR aims to add TestSpec events when executing the assertion engine.
Fixes
Checklist