feat: transaction run crud and execution #1465

mathnogueira · 2022-11-08T16:32:16Z

This PR adds endpoints for handling transaction runs and to execute a transaction

Checklist

tested locally
added new dependencies
updated the docs
added a test

schoren

The basic idea of the runner looks fine, but there are a few things that needs to be fixed for the app to keep consistent behaviour (such as order of things) and performance

server/executor/transaction_runner.go

schoren · 2022-11-08T18:42:54Z

server/executor/transaction_runner.go

+}
+
+func (r PersistentTransactionRunner) waitTestRunIsFinished(ctx context.Context, testRun model.Run) (model.Run, error) {
+	ticker := time.NewTicker(r.checkTestStateDelay)


this is not a great mechanism. Everything is within the same process here, so you could easily use channels to have a reactive model. This would yield 2 benefits:

instant updates, instead of depending on the polling interval. This also means more efficiency, since you just get the updates you need.

No r.db.GetRun, you can use the channel to transmit the updated test run

Implemented it.

schoren · 2022-11-08T18:48:20Z

server/model/transactions.go

 )

+func (rs TransactionRunState) IsFinal() bool {


is it neccesary to have a separated type for transaction's run state? can't we share the same with the tests?

I prefer to have a different state because some states in the test state don't make sense for transactions, such as AWAITING_TRACE

server/executor/transaction_runner.go

schoren · 2022-11-08T19:00:29Z

server/testdb/transaction_runs.go

+		return model.TransactionRun{}, fmt.Errorf("failed to marshal transaction steps: %w", err)
+	}
+
+	jsonStepRuns, err := json.Marshal(transactionRun.StepRuns)


this looks like it's gonna hold A LOT of data that already exists on the test_runs table. I would rather see an FK here.

Now it keeps simpler objects containing only the essentials for running the transaction

schoren · 2022-11-08T19:15:25Z

server/model/environments.go

+
+type (
+	Environment struct {
+		ID          string


Where is this being persisted?

schoren · 2022-11-08T19:18:11Z

server/model/environments.go

+		Name        string
+		Description string
+		CreatedAt   time.Time
+		Values      []EnvironmentValue


shouldn't this be a map[string]string? maybe an OrderedMap?

The order of environments doesn't matter, so no need for OrderedMap. map[string]string might be a better alternative than an array.

schoren · 2022-11-08T19:22:34Z

server/model/environments.go

+}
+
+func (e Environment) Merge(env Environment) Environment {
+	values := make(map[string]string)


I guess the idea of this is to avoid duplicated env keys. but in doing this, it's loosing the order guaranteed in a slice, which I imagine is the point of e.Values being a slice.

The only way to keep order and uniqueness of a map is using OrderedMap, even if the code looks a bit to verbose

yeah, the order doesn't matter in this case. I'll change it to map[string]string in a different PR

schoren · 2022-11-08T19:23:54Z

server/model/environments_test.go

+	assert.Contains(t, newEnv.Values, model.EnvironmentValue{Key: "URL", Value: "http://localhost"})
+	assert.Contains(t, newEnv.Values, model.EnvironmentValue{Key: "PORT", Value: "8085"})
+	assert.Contains(t, newEnv.Values, model.EnvironmentValue{Key: "apiKey", Value: "abcdef"})
+	assert.Contains(t, newEnv.Values, model.EnvironmentValue{Key: "apiKeyLocation", Value: "header"})


it would be nice to have an override key in this test

Also, if the idea of having an array is to keep the order, it would be great to have a test to guarantee the ordering

schoren · 2022-11-08T19:25:51Z

server/testdb/transactions_test.go

@@ -233,82 +233,3 @@ func TestGetTransactionsWithMultipleVersions(t *testing.T) {
 		assert.Equal(t, 2, test.Version)
 	}
 }
-
-// func TestTransactionSummary(t *testing.T) {


can all this commented code be deleted?

I forgot to remove it in my old PR. We don't have transaction summaries yet, so I decided to remove it and reintroduce it later.

api/openapi.yaml

jorgeepc · 2022-11-08T19:40:35Z

api/openapi.yaml

+    get:
+      tags:
+        - api
+      parameters:


It would be helpful to have the take and skip params here to support pagination.

makes sense. I'll add this in another PR, this is way too big already

schoren · 2022-11-10T20:25:43Z

server/model/transactions.go

+
+	TransactionRunState string
+
+	transactionStep struct {


Suggested change

transactionStep struct {

TransactionStep struct {

schoren · 2022-11-10T20:26:01Z

server/model/transactions.go

 func (t Transaction) HasID() bool {
 	return t.ID != ""
 }
+
+func NewTransactionRun(transaction Transaction) TransactionRun {
+	testIds := make([]transactionStep, 0, len(transaction.Steps))


Suggested change

testIds := make([]transactionStep, 0, len(transaction.Steps))

testIds := make([]TransactionStep, 0, len(transaction.Steps))

schoren · 2022-11-10T20:26:18Z

server/model/transactions.go

+	testIds := make([]transactionStep, 0, len(transaction.Steps))
+
+	for _, test := range transaction.Steps {
+		testIds = append(testIds, transactionStep{ID: test.ID, Name: test.Name})


Suggested change

testIds = append(testIds, transactionStep{ID: test.ID, Name: test.Name})

testIds = append(testIds, TransactionStep{ID: test.ID, Name: test.Name})

schoren

Looks awesome! thanks for addressing everything!

This reverts commit 784b0c7.

This reverts commit adbe84b.

This reverts commit 3beeff2.

mathnogueira force-pushed the feat/transaction-run branch from e0bf06e to cfdb822 Compare November 8, 2022 16:58

mathnogueira requested review from xoscar, schoren and jorgeepc November 8, 2022 18:43

schoren requested changes Nov 8, 2022

View reviewed changes

jorgeepc reviewed Nov 8, 2022

View reviewed changes

mathnogueira force-pushed the feat/transaction-run branch from cfdb822 to ce593e6 Compare November 9, 2022 14:40

mathnogueira requested a review from schoren November 10, 2022 19:38

schoren reviewed Nov 10, 2022

View reviewed changes

schoren approved these changes Nov 10, 2022

View reviewed changes

xoscar approved these changes Nov 11, 2022

View reviewed changes

mathnogueira added 17 commits November 11, 2022 16:07

add transaction run to openapi spec

512f0e8

implement transaction runs in db

837c4c9

generate frontend types

96178d6

add controller implementation

d54d351

implement transaction runner

b1ab79a

fix frontend code errors

b889030

fix runner and add get endpoints for transaction runs

b77ca96

add delete endpoint and fix error handling in db layer

7b9f6d9

refactor time.sleep and make transaction check period configurable

2f17247

don't export transaction runner

c58efe2

refactor patchEnvironment function

7e29324

refactor condition for patching environment

ac05ee9

add env to be overriden in env merge test

6d43577

use run instead of runs in api definition

19a0408

improve memory usage

bc16803

use channel to notify end of test run

5329546

export transaction step

57fead9

mathnogueira added 3 commits November 11, 2022 16:07

Revert "generate frontend types"

582218f

This reverts commit 784b0c7.

Revert "fix frontend code errors"

7a8a87f

This reverts commit adbe84b.

test consuming channel

82191d1

mathnogueira force-pushed the feat/transaction-run branch from adf0f2c to 82191d1 Compare November 11, 2022 19:17

mathnogueira added 5 commits November 11, 2022 17:24

fix run result channel

d3bc4a2

disable cleanup for debugging

3beeff2

remove hack

6e07792

fix channel

46e5d3d

Revert "disable cleanup for debugging"

30fae8a

This reverts commit 3beeff2.

mathnogueira merged commit aaad0b7 into main Nov 14, 2022

mathnogueira deleted the feat/transaction-run branch November 14, 2022 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: transaction run crud and execution #1465

feat: transaction run crud and execution #1465

mathnogueira commented Nov 8, 2022

schoren left a comment

schoren Nov 8, 2022

mathnogueira Nov 10, 2022

schoren Nov 8, 2022

mathnogueira Nov 9, 2022 •

edited

schoren Nov 8, 2022

mathnogueira Nov 10, 2022

schoren Nov 8, 2022

schoren Nov 8, 2022

mathnogueira Nov 9, 2022

schoren Nov 8, 2022

mathnogueira Nov 9, 2022

schoren Nov 8, 2022

schoren Nov 8, 2022

schoren Nov 8, 2022

mathnogueira Nov 9, 2022

jorgeepc Nov 8, 2022

mathnogueira Nov 10, 2022

schoren Nov 10, 2022

schoren Nov 10, 2022

schoren Nov 10, 2022

schoren left a comment

	testIds := make([]transactionStep, 0, len(transaction.Steps))
	testIds := make([]TransactionStep, 0, len(transaction.Steps))

	testIds = append(testIds, transactionStep{ID: test.ID, Name: test.Name})
	testIds = append(testIds, TransactionStep{ID: test.ID, Name: test.Name})

feat: transaction run crud and execution #1465

feat: transaction run crud and execution #1465

Conversation

mathnogueira commented Nov 8, 2022

Checklist

schoren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathnogueira Nov 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schoren left a comment

Choose a reason for hiding this comment

mathnogueira Nov 9, 2022 •

edited