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

[PoC] Parse exported script options with lib.TestPreInitState access #3000

Closed
wants to merge 2 commits into from

Conversation

na--
Copy link
Member

@na-- na-- commented Mar 31, 2023

This is built on top of #2999 and is a PoC of how we can parse exported script options while we have access to the *lib.TestPreInitState.

Because of the changes in #2999, JavaScript modules also have access to the *lib.TestPreInitState, so it can be used as a central place to coordinate things. For example, JS modules could presumably "register" scenario options specific to them, which could then be used when parsing and validating the options.

This is not intended for merging as it is, even though it slightly improves the UX (multiple errors when parsing the script options can now be returned at the same time), it's just a PoC that could be used for discussion. There are plenty of unresolved issues with it! For example, it doesn't handle .json config files or options that are in a .tar archive's metadata.json file. Both of these could presumably be made to work in a similar way, since we have the *lib.TestPreInitState in both cases, but it would require further refactoring.

The important part of this PoC is being able to statefully parse JSON files, having access to some k6-specific context while we do it, without fully rewriting the whole config. That said, the complexity overheads are somewhat big, so I am not sure if this is the way we want to go ahead in resolving #883, probably not... 😞

@na-- na-- changed the title [PoC] Parse exported script options with lib.TestPreInitState access [PoC] Parse exported script options with lib.TestPreInitState access Mar 31, 2023
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (pre-init-state-in-vu-init@ed32098). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 3157ce1 differs from pull request most recent head db2fb08. Consider uploading reports for the commit db2fb08 to get more accurate results

@@                     Coverage Diff                      @@
##             pre-init-state-in-vu-init    #3000   +/-   ##
============================================================
  Coverage                             ?   76.95%           
============================================================
  Files                                ?      228           
  Lines                                ?    17089           
  Branches                             ?        0           
============================================================
  Hits                                 ?    13150           
  Misses                               ?     3089           
  Partials                             ?      850           
Flag Coverage Δ
ubuntu 76.95% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Base automatically changed from pre-init-state-in-vu-init to master April 4, 2023 07:44
@na-- na-- closed this Apr 6, 2023
@na-- na-- deleted the options-with-pre-init-context branch April 6, 2023 15:41
imiric pushed a commit that referenced this pull request Apr 26, 2023
This is a quick and possibly naive change to support browser options per
scenario (#3022).

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.
imiric pushed a commit that referenced this pull request Apr 26, 2023
This is a quick and possibly naive change to support browser options per
scenario (#3022).

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.
@imiric imiric mentioned this pull request Apr 26, 2023
imiric pushed a commit that referenced this pull request Apr 28, 2023
This is a quick and possibly naive change to support browser options per
scenario (#3022).

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.
imiric pushed a commit that referenced this pull request Apr 28, 2023
This is a quick and possibly naive change to support browser options per
scenario (#3022).

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.
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