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

feat: display script checks in Validator #133

Merged
merged 9 commits into from
Sep 10, 2024

Conversation

cristianoventura
Copy link
Collaborator

@cristianoventura cristianoventura commented Sep 9, 2024

This PR implements a function that listens for the script:check event and populates the new Check tab on the Validator component. Ticket: https://github.com/grafana/k6-cloud/issues/2822

  • Run a script, such as the one in the snippet below
  • Go to the Checks tab and validate that the checks are being grouped and displayed appropriately
image

script for testing

import http from 'k6/http';
import { check, group } from 'k6'

export default function () {
  let resp
  resp = http.get('https://test.k6.io');

  check(resp, {
    "is status 200": (r) => r.status === 200,
  });

  group('group 1', function () {
    resp = http.get('https://test.k6.io');

    check(resp, {
      "is status 300": (r) => r.status === 300,
    });

    check(resp, {
      "is status 400": (r) => r.status === 400,
    });
  });

  group('group 2', function () {
    resp = http.get('https://test.k6.io');
    check(resp, {
      "is status 200": (r) => r.status === 200,
    });

    check(resp, {
      "is status 500": (r) => r.status === 500,
    });
  });
}

@cristianoventura cristianoventura self-assigned this Sep 9, 2024
Comment on lines +74 to +76
onScriptCheck: (callback: (data: K6Check[]) => void) => {
return createListener('script:check', callback)
},
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 not sure if this is expected or a bug but subgroups are not being sent to this channel script:check.

export default function () {
 ...
  group('group', function () {
    group('subgroup', function () {
      resp = http.get('https://test.k6.io');
      check(resp, {
        "is status 504": (r) => r.status === 504,
      });
    });
  });

Copy link
Member

Choose a reason for hiding this comment

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

bug on the logic, apparently groups can have another groups subsection that need to be checked 👍

@@ -0,0 +1,51 @@
import { K6Check } from '@/types'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Something is going on with syntax highlighting in Github for new files🤔 Any ideas what it could be?

Comment on lines +32 to +37
it('should group checks by path', () => {
const check1 = buildCheck({ path: '::check1' })
const check2 = buildCheck({ path: '::check2' })
const check3 = buildCheck({ path: '::group1::check3' })
const check4 = buildCheck({ path: '::group2::check4' })
const check5 = buildCheck({ path: '::group2::subgroup::check5' })
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Despite the script:check event not currently sending subgroups, the front-end is handling them.

Comment on lines 39 to 47
{hasFailures(check) && (
<Tooltip
content={`${getPassPercentage(check).toFixed(2)}% success rate`}
>
<Text truncate css={textStyles}>
Passed: {check.passes} | Failed: {check.fails}
</Text>
</Tooltip>
)}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if this is the best UX. Alternatively, we could display the percentage in the row and use the number of passes/fails in the tooltip.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO showing this info in the row is better - the checks tab doesn't have high information density, so hiding it isn't necessary.
Here's what we do in the plugin
image

And here's a screenshot from k6 cli (which actually makes me wonder if we need to show 100% success rate at all - there doesn't seem to be any value in that)
image

@cristianoventura cristianoventura marked this pull request as ready for review September 9, 2024 00:33

export function getPassPercentage(check: K6Check) {
const total = check.passes + check.fails
return (check.passes / total) * 100
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can passes + fails ever be 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 don't think so but, let me add a safe guard here.

@@ -148,6 +156,7 @@ export function Validator() {
<Tabs.Trigger value="logs">
Logs ({logs.length})
</Tabs.Trigger>
<Tabs.Trigger value="checks">Checks</Tabs.Trigger>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion: since checks are populated once the run is finished, maybe it's worth showing the number of checks similarly to how we show the number of logs - this will help the user understand whether they even need to look at that tab.

Here's how we show it in GCk6:
image

Comment on lines 39 to 47
{hasFailures(check) && (
<Tooltip
content={`${getPassPercentage(check).toFixed(2)}% success rate`}
>
<Text truncate css={textStyles}>
Passed: {check.passes} | Failed: {check.fails}
</Text>
</Tooltip>
)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO showing this info in the row is better - the checks tab doesn't have high information density, so hiding it isn't necessary.
Here's what we do in the plugin
image

And here's a screenshot from k6 cli (which actually makes me wonder if we need to show 100% success rate at all - there doesn't seem to be any value in that)
image

Copy link
Member

@Llandy3d Llandy3d 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, I agree with uladzimir suggestions!

is there a specific reason why passes/fails are shown for groups except default ? 🤔

@cristianoventura
Copy link
Collaborator Author

@Llandy3d @going-confetti Made changes based on the feedback. I think there's room for improvement especially around the collapsible component and the table header potentially being confusing depending on how many checks are being listed.

Let me know what you think.

Screen.Recording.2024-09-09.at.10.59.43.AM.mov

Copy link
Member

@Llandy3d Llandy3d left a comment

Choose a reason for hiding this comment

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

🚀 🚀 🚀

The :: separator is a k6 specific thing and I don't think I would use it in the UI as well 🤔

For the other things, I think it looks good, for the time we have to dedicate now it looks better than expected (the alternative being showing the end of test summary in a raw state) so I think that this PR is ready to be merged 🙌

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 dd3be42 into main Sep 10, 2024
1 check passed
@cristianoventura cristianoventura deleted the feat/display-script-checks branch September 10, 2024 11:55
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.

3 participants