Feat/support scenarios #89

w1kman · 2021-10-29T10:46:01Z

Support for converting several archives with `convert`

convert now accepts an array of archives. Every archive will become its own function in the produced script.

I've added one optional properties on the Log interface to expose some kind of control over the result

interface Log {
  ...
  exportAs?: String // Name of the function that wraps the created script (default: 'main')
}

legander

Cool that we can now generate multiple scenario scripts 🤩
Left a few comments and some questions as well. There is one particular piece that I think needs to be addressed. The execution flow of the persistedImports was for me at least somewhat difficult to follow, more difficult than it needs to be I think.

TL;DR
The following is not a requirement, I just want to pick you and the other reviewers brains a little bit and see what you think.

We have talked about this earlier in the week but I'm going to mention it here again because I encountered it again now when re-reviewing.
So.. one thing that stood out when testing the feature was that it was not so easily tested compared to other features since it requires multiple archives passed in an array rather than one single archive (which means we cannot just simply run har-to-k6 examples/multi-scenario.har).
I understand that code tries to be as non intrusive as possible to the HAR spec and existing implementation but I think conceptually it would make more sense to have one single data structure for grouping the scenarios since the options needs to be shared.

In the current PR implementation a test can be generated by one or more HARarchives. If multiple archives are passed then the options from the first archive will be the options which end up in the script. @w1kman please correct me if this is wrong.

I think if we instead merge the data into one unit it would make not only the implementation simpler but also would allow CLI support (which is most likely not need tho) but also make it conceptually clearer.

Configuration examples

Current PR implementation (separate archives)

- archive1.har (holds the scenario options)
- archive2.har

convert([archive1, archive2])

Proposition (combined data)

// single HAR file
{
   options: {} // script options, scenario options etc..
   archives: [
      { Scenario1HAR },
      { Scenario2HAR },
   ]
}
convert(archive)

src/convert.js

src/render/index.js

src/make.js

src/render/logic.js

allansson

Left a few comments.

I think that the global state needs to be fixed, before I can approve this. It's unlikely to cause any issues, but if it does it's gonna be a pain to figure out why.

li-har.spec.md

src/parse/exportAs.js

src/render/prettify.js

src/make.js

src/render/logic.js

package.json

legander

These are my change requests:

Move resolving logic to convert.js but abstracted into a function resolveImports or something fitting.
Only if this case is not already covered, add simple validation rule that prevents multiple default exports.

src/render/prettify.js

legander · 2021-11-12T12:51:17Z

src/render/root.js

+    options(main),
+    ...logicFunctions,
+  ]
+    .filter(item => item)
    .join(`\n\n`)
 }


Nice, this is in line with what I requested 👍 I think it should be happening earlier in the cycle though. Rather than having this logic in render I think it makes more sense to have it all the way up in convert.js making the flow:

normalize

validate

parse

resolveImports

render

I think it will require minimal changes since this added logic already works on the result from parse.
wdyt?

Come to think of it maybe it would make sense to not only validate the archives individually but also the set. If I'm not mistaken the current implementation would not complain if there were multiple archives configured to export default right?

Perhaps we should add a simple validation rule that checks the set then also runs each individual archive like today

function convert() { return normalize(archives, options) |> validate // validates both the set and also individual archives |> parse |> resolveImports |> render }

The general idea is to allow an advanced user to convert several HAR objects into one script (rather than have to convert them one by one and then stitch them together). The intended output of a "multi conversion" is multiple test scenarios, excluding options.

Rather than having this logic in render I think it makes more sense to have it all the way up in convert.js making the flow

Sure. The same reasoning should also apply for the function names (moving that as well).

Come to think of it maybe it would make sense to not only validate the archives individually but also the set.

I don't see a case where two archives could pass validation as a single but cause a fail as a set (unless we're building scenario options).

If I'm not mistaken the current implementation would not complain if there were multiple archives configured to export default right?

There can only be one default export per script (no matter the configuration).

Perhaps we should add a simple validation rule that checks the set then also runs each individual archive like today

I cannot come up with anything to validate unless we want to throw on colliding names (which are now resolved instead).

legander

Works well 👍 📦

allansson

Just a small question, but other than that I think it looks good.

allansson · 2021-12-01T09:49:04Z

src/render/prettify.js

+    semi: false,
    arrowParens: 'avoid',
    parser: 'babel',
    plugins: [babelParser],
+    singleQuote: true,
+    trailingComma: 'es5',
+    printWidth: 100,


This is technically a breaking change, as it means that the output will produce a different output from before. What was the reasoning behind it?

Produced k6 script should look more like examples on k6.io.

typings/main.d.ts

package.json

#### Improved - Handle reserved words (javascript and k6/har-to-k6) - Better char replacement - camelCase functions - prettier config updated to generate code with same styling as our example (docs) #### Removed - namedExport from li-har-spec

- __ITER - __VU - open

…sync function)

…ned.

allansson

LGTM! Great work! This is an awesome feature!

w1kman marked this pull request as ready for review October 29, 2021 12:01

w1kman requested review from EdvinasDaugirdas, legander and allansson October 29, 2021 12:01

legander requested changes Nov 5, 2021

View reviewed changes

src/convert.js Outdated Show resolved Hide resolved

src/render/index.js Outdated Show resolved Hide resolved

src/make.js Outdated Show resolved Hide resolved

src/render/logic.js Outdated Show resolved Hide resolved

allansson requested changes Nov 5, 2021

View reviewed changes

w1kman marked this pull request as draft November 5, 2021 13:44

w1kman marked this pull request as ready for review November 8, 2021 15:16

legander requested changes Nov 12, 2021

View reviewed changes

legander self-requested a review November 15, 2021 14:01

legander approved these changes Nov 15, 2021

View reviewed changes

allansson requested changes Dec 1, 2021

View reviewed changes

w1kman and others added 15 commits December 1, 2021 11:34

[fix] Add support for converting several HAR object (as scenarios)

5a7e9f7

[refactor] Re-format e2e (expected) code

d2bc9e8

[fix] Update li-har spec

bd52d0f

[fix] Update li-har spec (copy)

8196689

[fix] Handle scenario error

0c9ae21

[cr] Function name overhaul

e0fcd72

#### Improved - Handle reserved words (javascript and k6/har-to-k6) - Better char replacement - camelCase functions - prettier config updated to generate code with same styling as our example (docs) #### Removed - namedExport from li-har-spec

[cr] Add reserved words

aa35366

- __ITER - __VU - open

[chore] Add module.exports to typings/main.d.ts

a7d800d

[chore] Update typings/main.d.ts liHARToK6Script returns Promise (a…

a57dd68

…sync function)

[chore] Update typings/main.d.ts liHARToK6Script params were undefi…

fb9a6d0

…ned.

[cr] Change prettier.printWidth to 100

d119c1b

[cr] Move and abstract

b07c8b2

[fix] merge test-e2e and test-e2e-adhoc scripts into one

5ec892e

[fix] Update test styles

32c2bc0

[fix] Update typings

55e92db

w1kman force-pushed the feat/support-scenarios branch from 6d21aee to 55e92db Compare December 1, 2021 11:09

allansson self-requested a review December 1, 2021 11:39

allansson approved these changes Dec 1, 2021

View reviewed changes

w1kman merged commit b009254 into master Dec 6, 2021

w1kman deleted the feat/support-scenarios branch December 6, 2021 10:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/support scenarios #89

Feat/support scenarios #89

w1kman commented Oct 29, 2021 •

edited

legander left a comment •

edited

allansson left a comment

legander left a comment

legander Nov 12, 2021

legander Nov 12, 2021

w1kman Nov 15, 2021 •

edited

legander left a comment

allansson left a comment

allansson Dec 1, 2021

w1kman Dec 1, 2021

allansson left a comment

Feat/support scenarios #89

Feat/support scenarios #89

Conversation

w1kman commented Oct 29, 2021 • edited

Support for converting several archives with convert

legander left a comment • edited

Choose a reason for hiding this comment

Configuration examples

allansson left a comment

Choose a reason for hiding this comment

legander left a comment

Choose a reason for hiding this comment

legander Nov 12, 2021

Choose a reason for hiding this comment

legander Nov 12, 2021

Choose a reason for hiding this comment

w1kman Nov 15, 2021 • edited

Choose a reason for hiding this comment

legander left a comment

Choose a reason for hiding this comment

allansson left a comment

Choose a reason for hiding this comment

allansson Dec 1, 2021

Choose a reason for hiding this comment

w1kman Dec 1, 2021

Choose a reason for hiding this comment

allansson left a comment

Choose a reason for hiding this comment

w1kman commented Oct 29, 2021 •

edited

Support for converting several archives with `convert`

legander left a comment •

edited

w1kman Nov 15, 2021 •

edited