Implement jmeter-to-k6 - Githubissues

bookmoons commented 5 years ago

Adds v1 implementation of the converter. Converts all elements listed in #1.

Closes #1.

robingustafsson commented 5 years ago

@bookmoons Fantastic! I'll start reviewing this. Will begin by trying to convert a bunch of JMeter tests.

robingustafsson commented 5 years ago

Ok, I've played around with the converter a bit. This first round of feedback is just based on what I experienced while using the tool, I haven't looked much at the code yet. Great job overall from what I've seen so far! 🎉

General

JMeter files in examples/ should be .jmx rather than .xml
Could we add a final stage to the code generation that prettifies the code, using "prettier" (https://github.com/prettier/prettier)?

Can we add detection if any of the following vars are used in the generated code, and if not remove them from global scope?:

const vus = ...
let url, opts, auth, r, regex, match, matches, extract, output
let csvPage = {}, csvColumns = {}
const constants = {}
const vars = {}
const files = {}

Could we import {group, sleep} from "k6" rather than doing import k6 from "k6" and using k6.group and k6.sleep throughout the generated code?
I see there's a mix of ', " and template literal strings. Could we always use " when not necessary to use template literals (ie. when there aren't any variable substitutions that need to happen)? Maybe this could be done as part of a code formatting/prettification step as mentioned above?

An empty test plan results in exception (see JMX: https://gist.github.com/robingustafsson/d1b14c38b961494845cd00354a51d9cf):

/Users/<snip>/jmeter-to-k6/src/expand.js:3
for (const item of array) {
                 ^
TypeError: array is not iterable
at expand (/Users/<snip>/jmeter-to-k6/src/expand.js:3:22)
at countVus (/Users/<snip>/jmeter-to-k6/src/render.js:35:10)
at render (/Users/<snip>/jmeter-to-k6/src/render.js:13:15)
at convert (/Users/<snip>/jmeter-to-k6/src/convert.js:5:10)
at Object.<anonymous> (/Users/<snip>/jmeter-to-k6/bin/jmeter-to-k6.js:18:16)
at Module._compile (internal/modules/cjs/loader.js:722:30)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:733:10)
at Module.load (internal/modules/cjs/loader.js:620:32)
at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
at Function.Module._load (internal/modules/cjs/loader.js:552:3)

JSON Path Assertion/Extraction

jsonpath doesn't seem to be included in the generated imports when using JSONExtractor (see JMX and generated JS: https://gist.github.com/robingustafsson/f847c27bee10d8c8c487bee00f0a8c14)

JSONPathAssertion doesn't seem to recognized when running the converter:


/Users/<snip>/jmeter-to-k6/src/element.js:48
if (!route[name]) throw new Error('Unrecognized element: ' + name)
                ^

Error: Unrecognized element: JSONPathAssertion at element (/Users//jmeter-to-k6/src/element.js:48:27) at module.exports.args (/Users//jmeter-to-k6/src/element.js:1:102) at elements (/Users//jmeter-to-k6/src/elements.js:17:43) at module.exports.args (/Users//jmeter-to-k6/src/elements.js:1:102) at Object.hashTree (/Users//jmeter-to-k6/src/element/hashTree.js:4:10) at element (/Users//jmeter-to-k6/src/element.js:50:21) at module.exports.args (/Users//jmeter-to-k6/src/element.js:1:102) at elements (/Users//jmeter-to-k6/src/elements.js:17:43) at module.exports.args (/Users//jmeter-to-k6/src/elements.js:1:102) at Object.ThreadGroup (/Users//jmeter-to-k6/src/element/ThreadGroup.js:16:17)


## Random Controller
* Only seems to generate 1 case (`case 0: ...`), and throws `Error: Unexpected random index: 1` for all other possible options (see JMX and generated JS: https://gist.github.com/robingustafsson/7eae7a30a438f074de8a1c36cdbaa99b)

## Thread group
* Could the `if (__VU >= max_vus && __VU <= max_vus) { ... } else throw new Error('Unexpected VU: ' + __VU)` code be removed if only 1 thread group is used? (see JMX and generated JS: https://gist.github.com/robingustafsson/16979a21fcedbff9766cee7a48dd9d4a)
    * Also, I think we can skip the `else throw new Error('Unexpected VU: ' + __VU)` in all cases
* If `forever` is not selected and instead an iteration count (`loops`) has been specified there should be a statement like this in the generated script:
```js
export default function() {
    if (__ITER < whatever_iteration_count_specified) {
        // All thread group code
    }
}

Stepping thread group

The stepping (target values in stages) is wrong and so is the duration values (see JMX and generated JS: https://gist.github.com/robingustafsson/1e15ca6f6fd2a79612ea8c485be9eb3e):
- Each target entry should increase by the specified stepping (in this case by 10 VUs every step)
- There should be ramp-up of 5s leading up to each new VU step level
- After reaching the max VUs (in this case 100), there should be a hold of 60s
- After the hold there should be a ramp down based on a specified VUs per X seconds
- duration values are all in "ms" where they should be "s"
- So instead of [{"target":10,"duration":"30ms"},{"target":10,"duration":"30ms"},{"target":10,"duration":"30ms"}] for a 30 VU max with 10 VU stepping every 30s with 5s ramp-up and a 60s hold at the top with a ramp down to 0 VUs with 5 VUs per second, it should generate [{"target":10,"duration":"5s"},{"target":10,"duration":"30s"},{"target":20,"duration":"5s"},{"target":20,"duration":"30s"},{"target":30,"duration":"5s"},{"target":30,"duration":"30s"},{"target":30,"duration":"60s"},{"target":0,"duration":"6s"}]

bookmoons commented 5 years ago

Really nice. Updating all of this.

bookmoons commented 5 years ago

These were really good bug reports. Everything but the SteppingThreadGroup is fixed/updated. Will be getting into that stepping logic tomorrow.

bookmoons commented 5 years ago

The SteppingThreadGroup is converting as specified. Also added all the test files to examples.

robingustafsson commented 5 years ago

Good stuff! I'll do a second review tomorrow.

robingustafsson commented 5 years ago

Sorry for the delay. Have now done a second pass after your fixes, found some more issues:

General

Add semicolons at end of lines when running prettier. I know it's a subjective matter, but we've used semicolons all over the docs and examples for k6 so would like to keep it consistent 🙂
I had to add the following to package.json to have it run ./bundle now:
```
"dependencies": {
"@babel/runtime": "^7.0.0-beta.55"
}
```
as I got: Error: Cannot find module '@babel/runtime/helpers/builtin/interopRequireDefault' (any idea why?)

If controller

When I have a statement like ${someVariable}=="Some string" in JMeter it converts to something like this in k6 JS (see JMX and generated JS: https://gist.github.com/robingustafsson/56cd5aef0e69ad25afbaf0bde1b572d7):
```
if (`${vars[`someVariable`]}=="Some string"` === "true") {
  ...
}
```
This is not going to execute correctly, it would have to be something like this to work:
```
if (eval(`"${vars[`someVariable`]}"=="Some string"`) === true) {
  ...
}
```
When running this the JSON extraction block (JSONPostProcessor) that is defined above the if-controller is also included inside the if-statement in the generated JS.

Stepping thread group

In the ramp-down the slope should be calculated and then set as one stage entry rather than one for every X VUs/s (see JMX and generated JS: https://gist.github.com/robingustafsson/e13dd7d2dd7a560dafda4e9b2cf36239). In the linked gist the ramp-down should be {target: 0, duration:"20s"} (20s = 100 VUs / 5 VUs per second) rather than:

{ target: 95, duration: "0s" },
{ target: 95, duration: "1s" },
{ target: 90, duration: "0s" },
{ target: 90, duration: "1s" },
{ target: 85, duration: "0s" },
{ target: 85, duration: "1s" },
{ target: 80, duration: "0s" },
{ target: 80, duration: "1s" },
{ target: 75, duration: "0s" },
{ target: 75, duration: "1s" },
{ target: 70, duration: "0s" },
{ target: 70, duration: "1s" },
{ target: 65, duration: "0s" },
{ target: 65, duration: "1s" },
{ target: 60, duration: "0s" },
{ target: 60, duration: "1s" },
{ target: 55, duration: "0s" },
{ target: 55, duration: "1s" },
{ target: 50, duration: "0s" },
{ target: 50, duration: "1s" },
{ target: 45, duration: "0s" },
{ target: 45, duration: "1s" },
{ target: 40, duration: "0s" },
{ target: 40, duration: "1s" },
{ target: 35, duration: "0s" },
{ target: 35, duration: "1s" },
{ target: 30, duration: "0s" },
{ target: 30, duration: "1s" },
{ target: 25, duration: "0s" },
{ target: 25, duration: "1s" },
{ target: 20, duration: "0s" },
{ target: 20, duration: "1s" },
{ target: 15, duration: "0s" },
{ target: 15, duration: "1s" },
{ target: 10, duration: "0s" },
{ target: 10, duration: "1s" },
{ target: 5, duration: "0s" },
{ target: 5, duration: "1s" },
{ target: 0, duration: "0s" }

bookmoons commented 5 years ago

Thanks for that. Have updated all of it.

Not sure what caused Babel requirement, but I added it as a dependency. The only package I've added to bundle was papaparse for the CSV update.

When I have a statement like ${someVariable}=="Some string" in JMeter it converts to something like this in k6 JS

I think this piece is following JMeter. The IfController has a special flag for the condition, it seemed to be the only one that has it.

Interpret Condition as Variable Expression?
If this is selected, then the condition must be an expression that evaluates to "true" (case is ignored).

(Expression here meaning a JMeter expression). If you uncheck that flag then it's meant to be JavaScript, and it should render with an eval.

When running this the JSON extraction block (JSONPostProcessor) that is defined above the if-controller is also included inside the if-statement in the generated JS.

I think this one is correct. Processors and assertions are supposed to apply to all descendants of the parent element. So it caches them up in the context and duplicates for all samplers in that section of the tree.

bookmoons commented 5 years ago

It looks like that Babel requirement comes from yaml, only necessary when bundling. So I think it's OK:

It runs on Node.js 6 and later with no external dependencies, and in browsers from IE 11 upwards using @babel/runtime (Note: not included in dependencies, install separately).

That package is used by JSONPathAssertion JSONPathExtractor to support YAML data in a response body.

robingustafsson commented 5 years ago

Ah, you're right about the IFController and var expression vs JS code. I'd missed unchecking that flag.

Regarding the post processors and assertions you're also correct that they apply to all samplers in the same scope. Think I found another issue here though:

It seems that even when I add the post-processor to be a child of the sampler, the post-processor is still applied to children of the grandparent which seems to go against the docs (see JMX and generated JS: https://gist.github.com/robingustafsson/82041fb0eb790bce3ddddedc79570332):

...to ensure that a post-processor is applied only to a particular sampler, add it as a child of the sampler.

bookmoons commented 5 years ago

It seems that even when I add the post-processor to be a child of the sampler, the post-processor is still applied to children of the grandparent

Think I know just where I did this. Looking into it.

bookmoons commented 5 years ago

Not what I thought it was, but I've just pushed a fix.

robingustafsson commented 5 years ago

Ok @bookmoons. I've looked through the code as well and I think it looks good; structure, style and tests. Well done, thanks for your work on this 👏Now, go claim the bounty!

PS. I sent you a message on k6 slack if you can have a look when you get a chance.

bookmoons commented 5 years ago

Very nice, thank you very much. Submitted a claim and will open slack.

grafana / jmeter-to-k6

Implement jmeter-to-k6 #2

General

JSON Path Assertion/Extraction

Stepping thread group

General

If controller

Stepping thread group