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

Add HAR converter options #694

Merged
merged 6 commits into from
Jul 6, 2018
Merged

Add HAR converter options #694

merged 6 commits into from
Jul 6, 2018

Conversation

na--
Copy link
Member

@na-- na-- commented Jul 6, 2018

This adds a new --options flag for injecting options in the HAR converter output script (and closes #690). It also adds new --min-sleep and --max-sleep flags that determine the minimum and maximum sleep times at the end of the iteration in the output script.

@na-- na-- changed the title Add function to inject options into the generated scripts from HAR files Add HAR converter options Jul 6, 2018
Copy link
Member

@robingustafsson robingustafsson left a comment

Choose a reason for hiding this comment

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

LGTM

// GetPrettyJSON is a massive hack that works around the fact that some
// of the null-able types used in Options are marshalled to `null` when
// their `valid` flag is false.
// TODO: figure out something better or at least use reflect to do it, that
Copy link
Member

Choose a reason for hiding this comment

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

In the future we could perhaps compare the keys from the user supplied JSON file + any options from the converter itself (like maxRedirects) with the keys in the options struct and only marshal the ones that have been specified (via some sort of intermediary data struct/container, like you use below for removing the null values). Maybe that's what you mean by "use reflect to do it".

Copy link
Member Author

Choose a reason for hiding this comment

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

Not exactly, we can just enumerate struct fields with reflect (in order, compared to the unordered map traversal in Go) and check each one for a nil or a false value of its Valid flag. That way we'll also be able to output invalid JSON, but valid JS object syntax like this (notice the lack of quotes around the key name):

{
    maxRedirects: 0
};

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't do it now because reflect is quite finicky and I couldn't be sure that I'd get all of the edge cases correctly..

Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -12,6 +12,7 @@ console.log(rnd)
```

* A new option `--no-vu-connection-reuse` lets users close HTTP `keep-alive` connections between iterations of a VU. (#676)
* New options were added to the HAR converter. You can set the minimum and maximum sleep time at the end of an iteration with the new `--min-sleep` and `--max-sleep` CLI flags of `k6 convert`. You can also specify a JSON file with [script options](https://docs.k6.io/docs/options) that would be added to the options of the generated scripts with the new `--options` flag. (#694)
Copy link
Member

Choose a reason for hiding this comment

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

I'd separate the --min-sleep/--max-sleep and --options into two separate items.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done: 8660bad

Copy link
Contributor

@luizbafilho luizbafilho left a comment

Choose a reason for hiding this comment

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

LGTM

@robingustafsson robingustafsson merged commit ae79c5e into master Jul 6, 2018
@na-- na-- deleted the har-inject-options branch July 6, 2018 14:16
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.

Add an --options flag to k6 convert
3 participants