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

feature(backend): Required Gates #2888

Merged
merged 6 commits into from
Jul 11, 2023

Conversation

xoscar
Copy link
Collaborator

@xoscar xoscar commented Jul 7, 2023

This PR adds the capability for the server to handle required gates for test runs.

Required gates are a list of checkpoints where the test runner can be configured to mark the execution as failed or passed, without stopping the state machine steps.

Changes

  • Adds test runner resource
  • Includes required gates logic for the test runner
  • Adds migrations for the database
  • Includes mappings and persistence of data for test runs

Fixes

Checklist

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

https://www.loom.com/share/2d16fb9a483e4becb75832a2e591d10c

@xoscar xoscar self-assigned this Jul 7, 2023
@xoscar xoscar changed the base branch from main to feat/required-gates July 7, 2023 22:09
@xoscar xoscar added the backend label Jul 10, 2023
@xoscar xoscar marked this pull request as ready for review July 10, 2023 18:50
@xoscar xoscar requested review from schoren, mathnogueira, danielbdias and jorgeepc and removed request for mathnogueira July 10, 2023 18:50
@@ -0,0 +1,51 @@
openapi: 3.0.0
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 went with the proposed option @danielbdias suggested of creating a new test runner resource, I think this will allow us to have a more flexible way of dealing with customizable options when executing a test.

@@ -152,6 +160,14 @@ func (r persistentRunner) Run(ctx context.Context, testObj test.Test, metadata t

r.listenForStopRequests(ctx, cancelCtx, run)

// configuring required gates
if requiredGates != nil {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Required gates can be overwritten by the clients when executing a test run and if it's not part of the request we fallback to the global configuration

return r
}

func (r Run) ConfigureRequiredGates(gates []testrunner.RequiredGate) Run {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This logic is to support side effects from the linter and analyzer results to update the gates.
It also includes logic to generate the results from a test run without it for retro-compat

@@ -31,14 +32,18 @@ type RunResult struct {
}

type Runner interface {
Run(context.Context, test.Test, test.RunMetadata, environment.Environment) test.Run
Run(context.Context, test.Test, test.RunMetadata, environment.Environment, *testrunner.RequiredGates) test.Run
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 was thinking of moving both the env and test runner gates to an options struct, but I think this will be something that will change soon as part of the event-driven logic we might be applying for the next big September update 👁️

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 great 💥

server/app/app.go Outdated Show resolved Hide resolved
server/migrations/27_add_test_runner.down.sql Show resolved Hide resolved
Copy link
Collaborator

@schoren schoren left a comment

Choose a reason for hiding this comment

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

My main request change is to remove the reference to resourcemanager for the unused list of operations. The rest are readability suggestions

server/executor/testrunner/testrunner_entities.go Outdated Show resolved Hide resolved
server/executor/testrunner/testrunner_entities.go Outdated Show resolved Hide resolved
server/executor/testrunner/testrunner_repository.go Outdated Show resolved Hide resolved
server/executor/runner.go Outdated Show resolved Hide resolved
@xoscar xoscar requested a review from schoren July 11, 2023 15:36
@xoscar
Copy link
Collaborator Author

xoscar commented Jul 11, 2023

@schoren thanks for your review! I think I addressed all of your comments, I'm going to merge this to the feature branch so we can move forward

@xoscar xoscar merged commit 532e0fb into feat/required-gates Jul 11, 2023
25 checks passed
@xoscar xoscar deleted the feat/required-gates-backend branch July 11, 2023 19:08
xoscar added a commit that referenced this pull request Jul 13, 2023
* feature(backend): Required Gates (#2888)

* feature(backend): Required Gates

* adding tests and fixing bugs

* adding tests and fixing bugs

* addressing PR comments

* addressing PR comments

* updating go.modsum

* feature(frontend): Required Gates (#2932)

* feature(frontend): Test Runner Configuration

* feature(frontend): cleanup

* feature(frontend): removing unnecessary prop

* feature(frontend): addressing PR review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants