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(frontend): Required Gates #2932

Merged
merged 4 commits into from
Jul 12, 2023

Conversation

xoscar
Copy link
Collaborator

@xoscar xoscar commented Jul 12, 2023

This PR includes changes for the FE and backend to support the required gates feature

Changes

  • Adds the Test runner and Required Gates Result models
  • Encapsulates the status icon into a single component
  • Updates backend to support error messaging for failure gates from any test
  • Displays tooltips for test run results
  • Includes Test Run settings page and logic

Fixes

Checklist

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

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

@xoscar xoscar requested a review from jorgeepc July 12, 2023 19:01
@xoscar xoscar self-assigned this Jul 12, 2023
@xoscar xoscar marked this pull request as ready for review July 12, 2023 19:06
@@ -86,3 +85,5 @@ components:
type: integer
fail:
type: integer
allStepsRequiredGatesPassed:
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 can be used by clients to validate if all the gates from the steps passed

I was going back and forth on having a full required gates result at the transaction level.
But I think transactions should not care about exactly which gates passed or not, just if the steps ran correctly

@@ -407,5 +422,13 @@ func (td *RunRepository) readRunRow(row scanner) (TransactionRun, error) {
r.Fail = int(fail.Int32)
}

// checks if the flag exists, if it doesn't we use the fail field to determine if all steps passed
if allStepsRequiredGatesPassed == 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.

If the value doesn't exist int the DB we can generate based on the fail count as it was the same result we used in the past

state: TestRun['state'];
requiredGatesResult: RequiredGatesResult;
}

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 component encapsulates the run status icon that should be used everywhere in the app

@@ -0,0 +1,53 @@
import {useCallback, useEffect} from 'react';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding the settings form for the test runner options

required,
failed,
passed,
requiredFailedGates: required.filter(gate => failed.includes(gate)),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Calculates the failed gates that are required from the model so we can use them elsewhere

@@ -63,6 +64,15 @@ const ConfigEndpoint = (builder: TTestApiEndpointBuilder) => ({
providesTags: () => [{type: TracetestApiTags.SETTING, id: ResourceType.AnalyzerType}],
transformResponse: (rawLinter: TRawLinter) => Linter(rawLinter),
}),
getTestRunner: builder.query<TestRunner, unknown>({
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 we can simplify the endpoints for settings and the overall resources, we can think about a clever way to handle that in the future

Copy link
Contributor

Choose a reason for hiding this comment

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

Like that!

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.

Great work! Added a couple of comments

Comment on lines 32 to 34
<Typography.Text>
{ToTitle(gate)} <TooltipQuestion margin={6} title={SupportedRequiredGatesDescription[gate]} />
</Typography.Text>
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use a label here

@@ -22,6 +22,7 @@ export const TRACE_DOCUMENTATION_URL =
export const ADD_TEST_URL = 'https://docs.tracetest.io/web-ui/creating-tests';
export const ADD_TEST_OUTPUTS_DOCUMENTATION_URL = 'https://docs.tracetest.io/web-ui/creating-test-outputs';
export const ANALYZER_DOCUMENTATION_URL = 'https://docs.tracetest.io/analyzer/concepts';
export const TEST_RUNNER_DOCUMENTATION_URL = 'https://docs.tracetest.io/analyzer/concepts';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change this now?

export const SupportedRequiredGatesDescription = {
[SupportedRequiredGates.AnalyzerScore]: 'Test Runs will be marked as failed if the Analyzer Score is below the configured threshold',
[SupportedRequiredGates.AnalyzerRules]: 'Test Runs will be marked as failed if the Error Level Analyzer Rules are not met',
[SupportedRequiredGates.TestSpecs]: 'Test Runs will be marked as failed if on of the defined Test Specs fail',
Copy link
Contributor

Choose a reason for hiding this comment

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

Test Runs will be marked as failed if one of ?

@@ -63,6 +64,15 @@ const ConfigEndpoint = (builder: TTestApiEndpointBuilder) => ({
providesTags: () => [{type: TracetestApiTags.SETTING, id: ResourceType.AnalyzerType}],
transformResponse: (rawLinter: TRawLinter) => Linter(rawLinter),
}),
getTestRunner: builder.query<TestRunner, unknown>({
Copy link
Contributor

Choose a reason for hiding this comment

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

Like that!

@xoscar xoscar merged commit d13dd10 into feat/required-gates Jul 12, 2023
6 of 7 checks passed
@xoscar xoscar deleted the feat/required-gates-frontend branch July 12, 2023 21:55
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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants