Add test execution review functionality #77
Conversation
backend/test_observer/controllers/test_executions/test_executions.py
Outdated
Show resolved
Hide resolved
| "'UNDECIDED', " | ||
| "'MARKED_AS_FAILED', " | ||
| "'APPROVED_INCONSISTENT_TEST_DEF', " | ||
| "'APPROVED_UNSTABLE_PHSYICAL_INFRA', " |
There was a problem hiding this comment.
| "'APPROVED_UNSTABLE_PHSYICAL_INFRA', " | |
| "'APPROVED_UNSTABLE_PHYSICAL_INFRA', " |
| "CREATE TYPE testexecutionreviewstatus AS " | ||
| "ENUM( " | ||
| "'UNDECIDED', " | ||
| "'MARKED_AS_FAILED', " |
There was a problem hiding this comment.
| "'MARKED_AS_FAILED', " | |
| "'FAILED', " |
There was a problem hiding this comment.
Or just REJECTED? Cause right now TestExecution already has a status field that can be FAILED. So having another review_status field that can also be FAILED would be confusing
There was a problem hiding this comment.
I also think REJECTED would be better alternative to distinguish from other status types
|
|
||
| def upgrade() -> None: | ||
| op.execute( | ||
| "CREATE TYPE testexecutionreviewstatus AS " |
There was a problem hiding this comment.
| "CREATE TYPE testexecutionreviewstatus AS " | |
| "CREATE TYPE review_status AS " |
There was a problem hiding this comment.
(Are there other review statuses expected?)
There was a problem hiding this comment.
According to the Spreadsheet document I created, these were the ones added there.
There was a problem hiding this comment.
I meant this in context of renaming this type as just "review_status"
| op.execute( | ||
| "CREATE TYPE testexecutionreviewstatus AS " | ||
| "ENUM( " | ||
| "'UNDECIDED', " |
There was a problem hiding this comment.
| "'UNDECIDED', " | |
| "'UNKNOWN', " |
There was a problem hiding this comment.
Actually you don't need this state at all since it's to be used as a list (an empty list would be an "unknown" state)
There was a problem hiding this comment.
Good point, will follow-up with removing it.
| "'APPROVED_INCONSISTENT_TEST_DEF', " | ||
| "'APPROVED_UNSTABLE_PHSYICAL_INFRA', " | ||
| "'APPROVED_FAULTY_HARDWARE', " | ||
| "'APPROVED_GENERIC' " |
There was a problem hiding this comment.
| "'APPROVED_GENERIC' " | |
| "'APPROVED_TESTS_PASSED " |
| ) | ||
| op.execute( | ||
| "ALTER TABLE test_execution ADD COLUMN " | ||
| "review_status testexecutionreviewstatus[] NOT NULL " |
There was a problem hiding this comment.
| "review_status testexecutionreviewstatus[] NOT NULL " | |
| "review_statuses review_status[] NOT NULL " |
There was a problem hiding this comment.
Unfortunately, the enum name must match the enum class name in Python, according to SQLAlchemy
| op.execute( | ||
| "ALTER TABLE test_execution ADD COLUMN " | ||
| "review_status testexecutionreviewstatus[] NOT NULL " | ||
| "DEFAULT '{UNDECIDED}'::testexecutionreviewstatus[]" |
There was a problem hiding this comment.
| "DEFAULT '{UNDECIDED}'::testexecutionreviewstatus[]" | |
| "DEFAULT '{UNKNOWN}'::review_status[]" |
There was a problem hiding this comment.
Actually on a closer thought this should be by default an empty list and you don't need an UNKNOWN state at all.
andrejvelichkovski
changed the title
There was a problem hiding this comment.
We should probably also handle the case of test execution reruns. Perhaps if a rerun does occur then it might be appropriate to empty both review_status and review_comment
backend/tests/controllers/test_executions/test_test_executions.py
Outdated
Show resolved
Hide resolved
|
Forgot to handle the re-run case. Will update the code again. |
This PR introduces a new backend endpoint for reviewing a test execution.
Our decision was that we should have review process for both Test Execution and the whole Artefact. The artefact signoff is already implemented, but we still need to have similar implementation for Test Execution.
Implementation
As part of this PR there are a few different updates:
review_statusandreview_comment)Database migrations
As part of this update, we need to apply one more migration to the database.
Testing
Automated tests were added and a set of manual tests were performed to verify the behaviour.