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

feat(sever): for test runs, split the failed state into more descriptive statuses #2310

Merged
merged 4 commits into from Apr 5, 2023

Conversation

schoren
Copy link
Collaborator

@schoren schoren commented Apr 4, 2023

This PR makes the TestRun state more accurate by providing a new set of statuses: TRACE_FAILED, TRIGGER_FAILED and ASSERTION_FAILED.

Changes

  • add new states and implement where applicable

Fixes

Checklist

  • tested locally
  • added new dependencies
  • updated the docs
  • added a test

@schoren schoren requested review from mathnogueira, danielbdias and jorgeepc and removed request for mathnogueira April 4, 2023 16:57
Copy link
Member

@mathnogueira mathnogueira left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor

@jorgeepc jorgeepc left a comment

Choose a reason for hiding this comment

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

Looks good. One question: with this approach, is it safe to assume that if I get a TRACE_FAILED state, the trigger stage was correct?

@mathnogueira
Copy link
Member

Looks good. One question: with this approach, is it safe to assume that if I get a TRACE_FAILED state, the trigger stage was correct?

yes. If the trigger fails, the test execution stops

@schoren
Copy link
Collaborator Author

schoren commented Apr 4, 2023

@jorgeepc exactly. The failed stage means that all the previous steps succedded

Copy link
Contributor

@jorgeepc jorgeepc left a comment

Choose a reason for hiding this comment

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

I think we need to update the OpenAPI specs.

@schoren
Copy link
Collaborator Author

schoren commented Apr 4, 2023

@jorgeepc good catch. updated

Copy link
Contributor

@danielbdias danielbdias left a comment

Choose a reason for hiding this comment

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

It sounds ok! Before merging it I have one comment about retro compatibility: should we write a migration or handle in separate the old status to avoid problems with early versions of Tracetest?

@@ -180,13 +180,20 @@ const (
RunStateCreated RunState = "CREATED"
RunStateExecuting RunState = "EXECUTING"
RunStateAwaitingTrace RunState = "AWAITING_TRACE"
RunStateFailed RunState = "FAILED"
Copy link
Contributor

Choose a reason for hiding this comment

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

Today we save this value on our database, right? How will we keep the retro compatibility with older Tracetest versions?
Should we map the old FAILED string as always ASSERTION_FAILED?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even writing a migration to fix it on the database?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wouldn't do a DB migration, it is too error prone. Fallbacking to another stata is better I think. Why do you suggest ASSERTION_FAILED? not that I have a better one, but to understand the logic of the choice.

@schoren schoren merged commit c186fbf into main Apr 5, 2023
27 checks passed
@schoren schoren deleted the update-failed-statuses branch April 5, 2023 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Differentiate between a trigger failed state vs a trace failed state in a test run
4 participants