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

chore: scaffold Thresholds UI behind feature flag #438

Merged
merged 7 commits into from
Jan 29, 2025

Conversation

cristianoventura
Copy link
Collaborator

@cristianoventura cristianoventura commented Jan 27, 2025

Description

This PR scaffolds the UI for the Thresholds feature. The only functionalities implemented in this PR are:

  • Adding and removing thresholds from a list
  • Mapping the "statistic" according to the selected "metric"
  • Mapping the mask of the "value" input according to the selected "metric"

Validation, code generation, and UI polishments will come after, including the list of URLs in the "URL" dropdown.

How to Test

  • Open a generator
  • Type the konami code
  • Toggle the thresholds feature
  • Click on the now visible Thresholds icon
  • Check the console.log as you update the form with a valid state

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (npm run lint) and all checks pass.
  • I have run tests locally (npm test) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Screenshots (if appropriate):

Screen.Recording.2025-01-27.at.7.27.47.AM.mov

Related PR(s)/Issue(s)

@cristianoventura cristianoventura self-assigned this Jan 27, 2025
Comment on lines +49 to +53
<ControlledSelect
options={[]}
control={control}
name={`thresholds.${index}.url`}
/>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The URL dropdown is still not functional for now

Comment on lines 74 to 75
<Dialog.Root open={open} onOpenChange={onOpenChange}>
<Dialog.Content
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'm using a dialog for now so I can get the remaining UI working. Any feedback is appreciated here if you think this isn't the best approach.

@cristianoventura cristianoventura marked this pull request as ready for review January 27, 2025 14:58
@cristianoventura cristianoventura requested a review from a team as a code owner January 27, 2025 14:58
@@ -24,7 +24,7 @@ export function DevModeDialog() {
<Dialog.Description>Control feature toggles</Dialog.Description>
<Flex direction="column" gap="2" my="4">
{Object.entries(features).map(([feature, enabled]) => (
<Text as="label" size="2" key="feature">
Copy link
Collaborator

Choose a reason for hiding this comment

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

👀 thanks!

onOpenChange: (open: boolean) => void
}

export function ThresholdsDialog({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thresholds are a part of the k6 script options so I'd expect to see them in the Test options popover. Perhaps we can make it a bit wider to accommodate the thresholds table?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Example with the popover width set to 640px:
image
It's tight, so might need a further increase

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've increased to 800. It looks good now for the Thresholds tab, although we may have to revisit the other tabs as their space/content ratio looks slightly larger IMO. It's all behind a feature flag so we can address it before promoting Thresholds to production when the time comes.

import { zodResolver } from '@hookform/resolvers/zod'
import {
Dialog,
Table,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you import Table from @/components/Table, it will have the same header cell style as the test variables and load profile ones

Comment on lines 87 to 92
{ label: 'Less than', value: '<' },
{ label: 'Less than or equal to', value: '<=' },
{ label: 'Greater than', value: '>' },
{ label: 'Greater than or equal to', value: '>=' },
{ label: 'Equal to', value: '===' },
{ label: 'Not equal to', value: '!==' },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe worth discussing with Product, but I think I might be easier to scan <= and >= than Less than or equal to and Greater than or equal to (it will also let us save some precious horizontal space)

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've used it as you suggested for now just to fit better in the Popover. I'll bring this up to Product in the sync tomorrow.

import { useCallback, useEffect } from 'react'
import { useForm, useFieldArray, FormProvider } from 'react-hook-form'
import { ThresholdRow } from './ThresholdRow'
import { uniqueId } from 'lodash-es'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit(because there's one more place that uses this import), but have you considered crypto.randomUUID()?

Comment on lines 90 to 92
Thresholds are global pass/fail criteria that you can configure k6
to use, that can act on any result metrics. Read more about
thresholds in the{' '}
Copy link
Collaborator

@going-confetti going-confetti Jan 27, 2025

Choose a reason for hiding this comment

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

To better match the tone in other tabs, this could be updated to:

Define pass/fail criteria for your test metrics. If the performance of the system under test does not meet the conditions, the test finishes with a failed status.

Copy link
Collaborator Author

@cristianoventura cristianoventura left a comment

Choose a reason for hiding this comment

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

@going-confetti Thanks for the quick review. I've addressed your suggestions and moved the feature inside the Test Options menu.

cc @Llandy3d as we had a brief chat about this on Slack

Comment on lines +6 to +7
width?: string
align?: 'start' | 'end' | 'center'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New props now that we have more than one Popover with different sizes

Comment on lines +20 to +21
align={isThresholdsEnabled ? 'center' : 'end'}
width={isThresholdsEnabled ? '800px' : '400px'}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Increased the width of the Popover when Thresholds is enabled

@going-confetti going-confetti self-requested a review January 28, 2025 09:43
Copy link
Collaborator

@going-confetti going-confetti left a comment

Choose a reason for hiding this comment

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

LGTM!

@cristianoventura cristianoventura merged commit c54f7c7 into main Jan 29, 2025
3 checks passed
@cristianoventura cristianoventura deleted the chore/scaffold-threshold branch January 29, 2025 14:56
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.

2 participants