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: extra validations for browser checks #942

Merged
merged 4 commits into from
Oct 7, 2024
Merged

Conversation

VikaCep
Copy link
Contributor

@VikaCep VikaCep commented Sep 25, 2024

To prevent execution errors, we're adding some extra validations, in particular, these properties are not allowed in the options object:

  • duration
  • vus > 1
  • iterations > 1

image
image
image

@VikaCep VikaCep requested a review from a team as a code owner September 25, 2024 14:56
@VikaCep VikaCep self-assigned this Sep 25, 2024
@VikaCep VikaCep requested a review from ckbedwell September 25, 2024 14:58
@VikaCep VikaCep marked this pull request as draft September 26, 2024 14:31
@VikaCep VikaCep marked this pull request as ready for review October 1, 2024 21:43
@VikaCep VikaCep requested review from nadiamoe and w1kman October 1, 2024 21:43
Copy link
Contributor

@w1kman w1kman left a comment

Choose a reason for hiding this comment

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

When entering values as one "should", validation works.
But it's also very easy to bypass the validation.

image
image
(Note: when valid values are used in the same way, the test passes)

IMO, duration, vus and iterations should be stripped before running the script (if they aren't already).
If anything, we should notify the users that the mentioned properties doesn't have any effect 🤷🏻

@ckbedwell
Copy link
Contributor

But it's also very easy to bypass the validation.

@VikaCep this comes back to what we discussing yesterday and could explore further if we could modify the k6 types that we pass to Monaco.

@VikaCep VikaCep force-pushed the scripts-validation branch from 2ff57d0 to 30031f3 Compare October 3, 2024 21:42
@VikaCep
Copy link
Contributor Author

VikaCep commented Oct 3, 2024

I enforced the validation to ensure that iterations and vus can only be either undefined or 1, addressing the potential bypass issue identified by Thomas.

Specifically, what I'd prefer to avoid is rejecting a script with {vus: 1, iterations: 1}, as those are perfectly valid.

We don't want to reject scripts with configurations like the one above, as Nadia pointed out. That's the reason I don't mark it as invalid when these properties are defined.

IMO, duration, vus and iterations should be stripped before running the script (if they aren't already).
If anything, we should notify the users that the mentioned properties doesn't have any effect 🤷🏻

I think that’s a valid point, but it might confuse some users who may not understand why those configurations aren’t taking effect, especially when copy/pasting scripts between k6 and SM. However, with clear messaging, we could mitigate that confusion. Personally, I believe marking it as a validation error is a good way to teach users that those values are not valid for a synthetic monitoring check and should not be wrongly set.

@VikaCep this comes back to what we discussing yesterday and could explore further if we could modify the k6 types that we pass to Monaco.

I agree—this is a great idea since it’s easy to expand and add new rules. We should investigate this further; I’m not sure how we can implement it yet, but it’s definitely worth exploring in a follow-up PR.

@VikaCep VikaCep requested a review from w1kman October 4, 2024 14:33
@peterschretlen
Copy link
Contributor

👍 this seems like a good 80/20 solution for the time being. Does seem like there's followup work we can do to tighten loopholes on both the frontend and backend.

Copy link
Contributor

@w1kman w1kman left a comment

Choose a reason for hiding this comment

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

LGTM! 👍🏻

@VikaCep VikaCep merged commit 1e3960d into main Oct 7, 2024
5 checks passed
@VikaCep VikaCep deleted the scripts-validation branch October 7, 2024 11:47
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.

5 participants