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] various enhancements (request body example data extraction, support for generating scenario tests and load tests out of the box, and much more) #11106

Merged

Conversation

mostafa
Copy link
Contributor

@mostafa mostafa commented Dec 13, 2021

This PR builds on top of the PR #10614 by @ideas-into-software and fixes an issue I found while reviewing the code, and that is the empty lines in the output script.

@agilob @wing328 @ideas-into-software I'd be happy to have your feedback.

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>
 * resolved merge conflicts
 * replaced deprecated dependency
`org.apache.commons.lang3.StringEscapeUtils`

Signed-off-by: Michael H. Siemaszko <mhs@into.software>
@@ -506,33 +634,47 @@ public void preprocessOpenAPI(OpenAPI openAPI) {
break;
}
}
} catch (NullPointerException e) {
} catch (NullPointerException ignored) {
Copy link
Contributor

@agilob agilob Dec 13, 2021

Choose a reason for hiding this comment

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

let's log this exception and learn what's broken

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

@@ -70,6 +70,10 @@ private static boolean nonempty(List<?> params) {
return params != null && params.size() > 0;
}

private static boolean nonempty(Map<?, ?> params) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nonEmpty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.


if (cgOperation.getHasVendorExtensions()
&& cgOperation.vendorExtensions.containsKey(X_OPERATION_DATAEXTRACT)
&& cgOperation.vendorExtensions.get(X_OPERATION_DATAEXTRACT) instanceof java.util.Map) {
Copy link
Contributor

Choose a reason for hiding this comment

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

instanceof java.util.Map, don't we have Map already imported to do intanceof Map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a boolean check.

Map<String, DataExtractSubstituteParameter> dataExtractSubstituteParams, HTTPRequestGroup requestGroup) {

if (!dataExtractSubstituteParams.isEmpty()) {
List<String> existingVariablesNames = requestGroup.variables.stream().map(v -> v.key)
Copy link
Contributor

@agilob agilob Dec 13, 2021

Choose a reason for hiding this comment

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

List<String> existingVariablesNames = requestGroup.variables.stream().map(v -> v.key)
      .collect(Collectors.toList());

Set<DataExtractSubstituteParameter> initializeVariables = dataExtractSubstituteParams.values().stream()
     .filter(p -> !existingVariablesNames.contains(toVarName(p.paramName))).collect(Collectors.toSet());

this filter condition will be O(1) fast if existingVariablesNames is a Set, when existingVariablesNames is a List filter will be O(n)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

Set<DataExtractSubstituteParameter> initializeVariables = dataExtractSubstituteParams.values().stream()
.filter(p -> !existingVariablesNames.contains(toVarName(p.paramName))).collect(Collectors.toSet());

if (!initializeVariables.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

check if (!initializeVariables.isEmpty()) { not required when using for-each loop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

check(request, {
{
let url = BASE_URL + `/pet/findByStatus?status=${status}`;

Copy link
Contributor

Choose a reason for hiding this comment

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

still triple empty lines :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I haven't regenerated the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, the pipeline passed on 5082527 when the sample project was out of date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was strange to me, too.



let request = http.del(url);

Copy link
Contributor

Choose a reason for hiding this comment

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

4 empty lines!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.


String operationId = operation.getOperationId();

boolean hasRequestBodyExample = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

initialisation of this can go to line 558

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An outer block needs this variable if I move it inside the if block.


final CodegenOperation cgOperation = super.fromOperation(path, method.name(), operation, null);

String operationId = operation.getOperationId();
Copy link
Contributor

Choose a reason for hiding this comment

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

this line can go a lot lower than here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

@@ -416,26 +512,54 @@ public void preprocessOpenAPI(OpenAPI openAPI) {

final Operation operation = methodOperation.getValue();
final PathItem.HttpMethod method = methodOperation.getKey();
OptionalInt operationGroupingOrder = OptionalInt.empty();
Copy link
Contributor

Choose a reason for hiding this comment

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

had no idea OptionalInt is a thing 👍

List<Parameter> bodyOrFormParams) {

Optional<Map.Entry<String, Example>> requestBodyExampleEntry = requestBody.getContent().get(contentTypeValue)
.getExamples().entrySet().stream().findFirst();
Copy link
Contributor

Choose a reason for hiding this comment

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

this chain map->set->stream->optional is quite slow, how often is this method call? might be worth to new ArrayList(getExamples().entrySet()).get(0)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only once per request. It doesn't matter much.

@@ -66,7 +66,11 @@
*
* @return true if parameter exists, false otherwise
*/
private static boolean nonempty(List<?> params) {
private static boolean nonEmpty(List<?> 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.

@agilob This is unrelated to our code, but I changed it nevertheless.

for (Map.Entry<?, ?> respExtensionEntry : respExtensions.entrySet()) {

if (X_OPERATION_RESPONSE_HIDE.equals(String.valueOf(respExtensionEntry.getKey()))) {
hideOperationResponse = Boolean.parseBoolean(respExtensionEntry.getValue().toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

should this have a break statement after setting this to true? effectively, now hideOperationResponse will have the last value of respExtensionEntry.getValue(), and if you want the last value, then we don't need a loop here, and since we're iterating on respExtensions.entrySet() a Set, the last value is not deterministic, so method might produce different value on every run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

Copy link
Contributor

@agilob agilob left a comment

Choose a reason for hiding this comment

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

nice, thanks 👍

@mostafa
Copy link
Contributor Author

mostafa commented Dec 14, 2021

@agilob Thanks to you for the meticulous review! Finally mergeable?

@agilob
Copy link
Contributor

agilob commented Dec 14, 2021

There is no technical commette for k6, so it now depends on when @wing328 or @jimschubert can merge it

inputSpec: modules/openapi-generator/src/test/resources/3_0/petstore-with-fake-endpoints-models-for-testing.yaml
templateDir: modules/openapi-generator/src/main/resources/k6
additionalProperties:
appName: PetstoreClient
Copy link
Member

Choose a reason for hiding this comment

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

There's already one in ./bin/configs/other/k6.yaml.

I'll remove it after merging this PR.

@wing328 wing328 changed the title [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) [K6 Generator] various enhancements (request body example data extraction, support for generating scenario tests and load tests out of the box, and much more) Dec 17, 2021
@wing328 wing328 added this to the 5.3.1 milestone Dec 17, 2021
@wing328 wing328 merged commit fedc54a into OpenAPITools:master Dec 17, 2021
@mostafa mostafa deleted the k6-openapi-extract-examples-enhancements branch December 17, 2021 09:43
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