feat: recording verification rule #128

Llandy3d · 2024-09-04T22:45:28Z

Closes https://github.com/grafana/k6-cloud/issues/2530

This PR adds the "backend" logic for the Recording Verification Rule.

This rule will add checks to compare that the status codes from the responses match the ones from the recording.
I've added this as a separate new rules because it's very specific and it seems to make sense but this is open for debate if it should be part of a bigger Verification rule that handles user defined verification + this global recording check 🤔

Llandy3d · 2024-09-04T22:55:06Z

src/utils/generator.ts

@@ -21,7 +22,7 @@ export function createNewGeneratorFile(recordingPath = ''): GeneratorFileData {
    testData: {
      variables: [],
    },
-    rules: [],
+    rules: [createEmptyRule('recording-verification')],


this rule will be present on every newly created generator as a default that can be removed

e-fisher

Sweet! 🤩 Works as advertised and adds checks with correct status codes 👌

Few comments:

Not a blocker but would be nice to have a test case covering this in codegen.test.ts
Should we terminate the test when check fails? Currently, there's no indication that checks failed, but I'm guessing it will be added as another task

Llandy3d · 2024-09-05T12:47:59Z

Sweet! 🤩 Works as advertised and adds checks with correct status codes 👌

Few comments:

Not a blocker but would be nice to have a test case covering this in codegen.test.ts

Should we terminate the test when check fails? Currently, there's no indication that checks failed, but I'm guessing it will be added as another task

According to k6 docs on checks they are not supposed to be make the test fail.

Regarding the missing inidication, that's because check information seems to be given only on end of test summary, I was already thinking that we might need a way to retrieve them and show in the Validator but we do not get them for free :(

e-fisher · 2024-09-05T13:03:34Z

According to k6 docs on checks they are not supposed to be make the test fail.

It makes in context of performance testing, but using Validator I would expect it to stop when something goes wrong, allowing me to see which request breaks it. But I see 2 possible solutions here:

Add a rule option: Stop execution when check fails [yes/no]
Don't stop execution but display an icon (⚠️) near requests which failed check with a hoverable tooltip showing the reason (expected status 200, got 403)

WDYT? Those suggestions are not to be part of that PR though, just ideas for future improvements.

Llandy3d · 2024-09-05T13:30:46Z

According to k6 docs on checks they are not supposed to be make the test fail.

It makes in context of performance testing, but using Validator I would expect it to stop when something goes wrong, allowing me to see which request breaks it. But I see 2 possible solutions here:

Add a rule option: Stop execution when check fails [yes/no]

Don't stop execution but display an icon (⚠️) near requests which failed check with a hoverable tooltip showing the reason (expected status 200, got 403)

WDYT? Those suggestions are not to be part of that PR though, just ideas for future improvements.

On the immediate future I think we might get checks information only at the end of the test run 🤔 This would exclude option 2 short-term

For option 1, I was looking into how to make them fail and I think we can combine them with Thresholds, possibly checking the rate of error for checks overall and expect all of them to pass, fail otherwise. With the setting like you mentioned could be a nice way to do it 🤔

e-fisher · 2024-09-05T13:40:33Z

For option 1, I was looking into how to make them fail and I think we can combine them with Thresholds, possibly checking the rate of error for checks overall and expect all of them to pass, fail otherwise. With the setting like you mentioned could be a nice way to do it 🤔

Yeah, that's what we do for e2e tests

    thresholds: {
      checks: [
        {
          threshold: 'rate == 1.00',
        },
      ],
    },

Llandy3d · 2024-09-05T14:47:41Z

Re-requesting review as I've added a way to retrieve checks information to showcase in the Validator.

There is one big issue, this can be brittle, if the user uses its own version of handleSummary this will break the script, retrieving this information via a custom version of it, it's a nice and quick way to get that information but we should consider having the ability to export logs of this kind without having to rely on the handleSummary :(

e-fisher

After looking into this some more - currently we are generating a check per every status code checked, it looks something like this:

       ✗ is status 200
        ↳  53% — ✓ 758 / ✗ 665
       ✗ is status 401
        ↳  26% — ✓ 14 / ✗ 38
       ✗ is status 204
        ↳  0% — ✓ 0 / ✗ 24
       ✗ is status 403
        ↳  0% — ✓ 0 / ✗ 23

Which might not be very helpful. Perhaps we should name the check "response status code matches recording" so all checks generated by verification rule are grouped under a single check? Another option would be adding request url to the check name, but that might generate too many checks 🤔

Llandy3d · 2024-09-06T10:07:33Z

src/rules/verification.ts

+
+  const verificationSnippet = `
+check(resp, {
+    'Recording Verification Rule: status matches recording': (r) => r.status === ${response.statusCode},


The checks are now grouped under a single name.

This still respects groups differences, in case you have two groups, it will be two checks

Llandy3d · 2024-09-06T10:08:48Z

src/script.ts

+  const handleSummaryIndex = scriptLines.findIndex(
+    (line) =>
+      // NOTE: if the custom handle summary is commented out we can still insert our snippet
+      // this check should be improved
+      line.includes('export function handleSummary(') && !line.includes('//')
+  )
+
+  // NOTE: checks works only if the user doesn't define a custom summary handler
+  // if no custom handleSummary is defined we add our version to retrieve checks
+  if (handleSummaryIndex === -1) {
+    scriptLines.push(checksSnippet)
+  }


We only add the checks retrieval logic via handleSummary if there is no custom summary defined by the user since we retrieve that information via summary

e-fisher

LGTM! 🙌

Llandy3d added 3 commits September 4, 2024 18:07

wip

c0acf1f

recording verification rule

d7aecb6

fix test

fa58287

Llandy3d requested review from going-confetti and e-fisher September 4, 2024 22:45

add recording verification rule on new generator

ce8f2eb

Llandy3d commented Sep 4, 2024

View reviewed changes

e-fisher approved these changes Sep 5, 2024

View reviewed changes

retrieve checks from k6

31af402

Llandy3d requested a review from e-fisher September 5, 2024 14:45

add test

a4ac996

e-fisher reviewed Sep 6, 2024

View reviewed changes

Llandy3d added 3 commits September 6, 2024 11:55

better handling of checks script insertion

a8142e0

unify rule checks under a single name

80f2241

fix test

df49153

Llandy3d commented Sep 6, 2024

View reviewed changes

Llandy3d requested a review from e-fisher September 6, 2024 10:08

e-fisher approved these changes Sep 6, 2024

View reviewed changes

Llandy3d merged commit 35241b6 into main Sep 6, 2024
1 check passed

Llandy3d deleted the verification branch September 6, 2024 10:35

github-actions bot mentioned this pull request Jan 14, 2025

chore(main): release 0.11.0 #405

Closed

croco-bot mentioned this pull request Jan 24, 2025

chore(main): release 0.13.0 #432

Merged

croco-bot mentioned this pull request Feb 13, 2025

chore(main): release 0.14.0 #489

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: recording verification rule #128

feat: recording verification rule #128

Llandy3d commented Sep 4, 2024 •

edited

Loading

Llandy3d Sep 4, 2024

e-fisher left a comment

Llandy3d commented Sep 5, 2024

e-fisher commented Sep 5, 2024

Llandy3d commented Sep 5, 2024

e-fisher commented Sep 5, 2024

Llandy3d commented Sep 5, 2024

e-fisher left a comment

Llandy3d Sep 6, 2024

Llandy3d Sep 6, 2024

e-fisher left a comment

feat: recording verification rule #128

feat: recording verification rule #128

Conversation

Llandy3d commented Sep 4, 2024 • edited Loading

Llandy3d Sep 4, 2024

Choose a reason for hiding this comment

e-fisher left a comment

Choose a reason for hiding this comment

Llandy3d commented Sep 5, 2024

e-fisher commented Sep 5, 2024

Llandy3d commented Sep 5, 2024

e-fisher commented Sep 5, 2024

Llandy3d commented Sep 5, 2024

e-fisher left a comment

Choose a reason for hiding this comment

Llandy3d Sep 6, 2024

Choose a reason for hiding this comment

Llandy3d Sep 6, 2024

Choose a reason for hiding this comment

e-fisher left a comment

Choose a reason for hiding this comment

Llandy3d commented Sep 4, 2024 •

edited

Loading