-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add JS scenario options #3036
Add JS scenario options #3036
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3036 +/- ##
==========================================
- Coverage 77.04% 77.02% -0.02%
==========================================
Files 229 229
Lines 17065 17065
==========================================
- Hits 13147 13144 -3
- Misses 3077 3079 +2
- Partials 841 842 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! but I would prefer to hear that this will actually be sufficient for the @grafana/k6-browser team.
941c830
to
3069173
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, let's wait for the browser team's feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Thanks for this 🙇
Previously, the new scenario `options` field added in #3036 was being serialized to JSON by default, which made it an incompatible change with some of our internal Cloud services that validate the existing configuration structure. This change makes the object optional, and it will only be (de)serialized if it's included in the data.
Previously, the new scenario `options` field added in #3036 was being serialized to JSON by default, which made it an incompatible change with some of our internal Cloud services that validate the existing configuration structure. This change makes the object optional, and it will only be serialized if it's included in the data.
Previously, the new scenario `options` field added in #3036 was being serialized to JSON by default, which made it an incompatible change with some of our internal Cloud services that validate the existing configuration structure. This change makes the object optional, and it will only be serialized if it's included in the data.
Previously, the new scenario `options` field added in #3036 was being serialized to JSON by default, which made it an incompatible change with some of our internal Cloud services that validate the existing configuration structure. This change makes the object optional, and it will only be serialized if it's included in the data.
Previously, the new scenario `options` field added in #3036 was being serialized to JSON by default, which made it an incompatible change with some of our internal Cloud services that validate the existing configuration structure. This change makes the object optional, and it will only be serialized if it's included in the data.
Previously, the new scenario `options` field added in #3036 was being serialized to JSON by default, which made it an incompatible change with some of our internal Cloud services that validate the existing configuration structure. This change makes the object optional, and it will only be serialized if it's included in the data.
This is a quick and possibly naive change to support browser options per scenario (#3022). These options will be parsed and validated by the browser module.
It doesn't try to enable generic options for any JS module as #3000 did, but it seems Ned abandoned this idea. We can open this discussion again once #883 is resolved.
I tested it with
k6 archive
, and it does serialize and deserialize frommetadata.json
properly. Should I add a test for this? An archive unit test would be a bit convoluted to add because of theExecutorConfig
interface, but maybe an integration test would be simpler.