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

[ K6 Generator ] Further K6 OpenAPI generator enhancements (request body example data extraction, support for generating scenario tests and load tests out of the box, and much more) #10614

Conversation

ideas-into-software
Copy link
Contributor

@ideas-into-software ideas-into-software commented Oct 17, 2021

Following up on the "parameter example data extraction" enhancement merged via #9750, this MR brings additional significant improvements to the K6 OpenAPI generator, i.e.:

  • request body example data extraction
  • request grouping and ordering
  • response visibility
  • request data extraction for chaining requests

Most of these are accomplished via custom OpenAPI extensions (https://swagger.io/docs/specification/openapi-extensions/) introduced, i.e.:

  • x-k6-openapi-operation-grouping – operation grouping - group operations and define their ordering; useful e.g. for automatically generated scenario tests
  • x-k6-openapi-operation-response – operation response - for now, allows to hide given operation response, so that in case of multiple 2xx responses, generated script checks only against e.g. code 200 responses
  • x-k6-openapi-operation-dataextract – extract data from operation - allows to specify path to value in body of response which should be extracted and assigned to variable for later use by other operations; useful e.g. for automatically generated load test scripts

Code is documented. Entire solution will be showcased as part of the upcoming EclipseCon 2021 session: "Automated testing of OpenAPI-described RESTful microservices utilizing open source tools" https://www.eclipsecon.org/2021/sessions/automated-testing-openapi-described-restful-microservices-utilizing-open-source-tools

In a nutshell, with these additional enhancements, it possible not only to specify all of the example data and use it in the ready-to-run generated script, but to also generate ready-to-run scenario tests as well as load tests.

This allows for integrating the entire solution into CI/CD pipeline, i.e. as an additional, post-automated-deployment step, running automated REST API testing via automatically generated smoke tests / load tests / scenario tests, generated off on latest OpenAPI spec.

Obviously, generating OpenAPI spec itself is out of the scope for this project – but can be easily integrated into pipeline via existing gradle/maven plugins (e.g. io.swagger.core.v3.swagger-gradle-plugin) and run in a separate, earlier, step of the pipeline. Also, for automated REST API testing as part of CI/CD pipeline, continuous deployment must be in place – i.e. latest version of application exposing such REST API must be first deployed.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

 * request body example data extraction
 * request grouping and ordering
 * response visibility
 * request data extraction for chaining requests

Signed-off-by: Michael H. Siemaszko <mhs@into.software>
 - regenerated samples

Signed-off-by: Michael H. Siemaszko <mhs@into.software>
@ideas-into-software ideas-into-software changed the title Further K6 OpenAPI generator enhancements (request body example data extraction, support for generating scenario tests and load tests out of the box, and much more) [ K6 Generator ] Further K6 OpenAPI generator enhancements (request body example data extraction, support for generating scenario tests and load tests out of the box, and much more) Oct 17, 2021
@wing328
Copy link
Member

wing328 commented Oct 18, 2021

@ideas-into-software thanks for the PR. Can you please merge the latest master into your branch and resolve the merge conflicts?

cc @mostafa

 * resolved merge conflicts
 * replaced deprecated dependency
`org.apache.commons.lang3.StringEscapeUtils`

Signed-off-by: Michael H. Siemaszko <mhs@into.software>
@ideas-into-software
Copy link
Contributor Author

@ideas-into-software thanks for the PR. Can you please merge the latest master into your branch and resolve the merge conflicts?

cc @mostafa

@wing328

Merge conflicts resolved: ed3f00c

@ideas-into-software
Copy link
Contributor Author

Entire solution will be showcased as part of the upcoming EclipseCon 2021 session: "Automated testing of OpenAPI-described RESTful microservices utilizing open source tools" https://www.eclipsecon.org/2021/sessions/automated-testing-openapi-described-restful-microservices-utilizing-open-source-tools

Example project: https://gitlab.com/oss-contrib/EclipseCon2021/example-project

Copy link
Contributor

@mostafa mostafa left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! 🙏
I made a few comments on formatting of the template, and thus the resulting k6 script.

let body = {"id": "long", "category": {"id": "long", "name": "string"}, "name": "string", "photoUrls": "list", "tags": "list", "status": "string"};

let params = {headers: {"Content-Type": "application/json", "Accept": "application/json"}};

Copy link
Contributor

Choose a reason for hiding this comment

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

@ideas-into-software
Too many new lines in between, please consider updating the mustache template. This also applies further down.

let params = {headers: {"api_key": `${apiKey}`, "Accept": "application/json"}};

// this is a DELETE method request - if params are also set, empty body must be passed
let request = http.del(url, {} , params);
Copy link
Contributor

@mostafa mostafa Oct 27, 2021

Choose a reason for hiding this comment

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

Formatting might be fixed: ..., {}, params).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @mostafa

Unfortunately, I have not found any feature in mustache which allows to suppress the new lines output from the mustache tags; I'm no mustache expert, but checked their docs, also some additional tips, but none of these produce the result required - i.e. having a mustache template that's readable (i.e. not all tags are on one line / they're split according to their purpose) and the resulting script not having extra new lines output.

If you wish please do take a look yourself, but please be informed that neither ~ nor ! work, as per info I found; the docs (https://mustache.github.io/mustache.5.html, https://mustache.github.io/mustache.1.html) do not even mention such options.

Perhaps this is something that needs to be configured in the mustache processor itself / some additional setting; someone who knows mustache from this side a bit better might have a clue.

Sorry I cannot help on this particular issue.

Copy link
Contributor

@mostafa mostafa Nov 2, 2021

Choose a reason for hiding this comment

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

I know of this issue and this is probably a trade-off between readability of the template and the intended results.
@wing328 Can you advise us on this particular issue? You definitely have more experience than both of us on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

You need to reformat the template so it looks ugly for us. In the mustache file on line 41 you will have something like:

{{/params}}{{^params}}

which is kinda equivalent to one line } else if { your current mustache formatting currently is equivalent to:

} else
     if   
      {

Copy link
Contributor

@agilob agilob Nov 3, 2021

Choose a reason for hiding this comment

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

You need to merge the mustache not (^check) tags into closing of previous tag at least.

Copy link
Contributor

Choose a reason for hiding this comment

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

@wing328 I suggest we merge it as is.

No... let's not merge code that can produce 20 empty lines in a row or 2 lines in a row with 8 spaces in them, really bad idea for tracibility.

@ideas-into-software I did exactly what I explained above and opened PR to your branch: DataInMotion#1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @agilob

I reviewed your PR (DataInMotion#1), testing all this locally. Please take into account the following:

1. I explicitly mentioned - what @mostafa also understands - that the mustache template becomes unreadable when all new lines between different kinds of tags is removed; what you did in your PR I already did while ago when working on this, but the result is what I mentioned earlier few times.

What you mentioned in your earlier comment is "You need to merge the mustache not (^check) tags into closing of previous tag at least." - that's what I was referring to in earlier comment, i.e. that this by itself does not change anything in terms of extra whitespace. Whitespace control character would be the solution, but this mustache processor does not support it (see my earlier comment).

2. I noticed you or someone else made changes to the K6ClientCodegen which is part of this PR you made - there are several issues with these changes:

a) @ #preprocessOpenAPI method

The original intent for swallowing that exception is that not all operations have parameters and there's absolutely nothing wrong with that - so if NullPointerException is thrown we can safely ignore it, instead of logging it, like you started logging it via LOGGER.error("{}", e.getMessage(), e);; this results in unecessary stack traces printed out in output when running the generator.

b) @ #shouldHideOperationResponse method

Intent for using switch / case is additional extensions / extension params which need to be handled differently (see getDataExtractSubstituteParameter method for comparison); for some reason someone removed that switch.

c) @ #calculateRequestOrder method

Signature of this method mentions Integer, but your refactor returns primitive int, i.e. autoboxing kicks in, which is a performance issue.

I'll gladly merge this PR you made but these would need to be addressed.

Copy link
Contributor

@agilob agilob Nov 11, 2021

Choose a reason for hiding this comment

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

  1. I explicitly mentioned - what @mostafa also understands - that the mustache template becomes unreadable when all new lines between different kinds of tags is removed; what you did in your PR I already did while ago when working on this, but the result is what I mentioned earlier few times.

Because this is how we develop templates, to produce generated code that is easy to read and understand. Generated code must be easy to follow, that's a profit here, but we're paying the price of having complex mustache templates. Check complexity of this template if you don't believe.

this results in unecessary stack traces printed out in output when running the generator.

The stacktraces are ABSOLUTELY necessary because they point out problems with the generator or configuration. You can change it to remove 3rd param from logging, but please replace logger to say which field is failing and why. I didn't have time to add this info, looks like you know more about this generator to fix the NPE problem correctly.

Signature of this method mentions Integer, but your refactor returns primitive int, i.e. autoboxing kicks in, which is a performance issue.

There is no performance issue. I replaced two Integer.valueOf() with one autoboxing on return statement. I put two ints in place of two Integers. Technically this is faster, but was not the point, it's simply less verbose to read that return statement. There are much bigger problems in openapi in performance and memory leaks.

If you're concerned about performance, don't put code like this this.isDelete = method.equals("delete") ? true : false; 👀

I'll gladly merge this PR you made but these would need to be addressed.

You said above:

Unfortunately, what @agilob suggested does not work as expected; what's odd is that it results in additional new line, instead of less new lines.

This PR is take it or leave it. I showed you how to write the template to prevent getting 4 empty lines every 2 lines. It can't be merged in current state where we don't know what is throwing NPE and when generated code will add +300 empty lines to my project. (Actually, these lines aren't empty, but contain 8 whitespaces, and that's even worse).

Copy link
Contributor

Choose a reason for hiding this comment

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

@agilob Thanks for the comments! I think it's nothing to worry about, as the issues we mentioned might be fixed. 🙂

@ideas-into-software Thanks for your contribution! 🙏 I'll take care of the rest to make this PR mergeable.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks, I'll be happy to test it on my project 👍

@wing328
Copy link
Member

wing328 commented Dec 17, 2021

Closed via #11106

@wing328 wing328 closed this Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants