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

[Erlang] add PropEr (property-based testing tool) support #1102

Merged
merged 10 commits into from
Oct 27, 2018
Merged

[Erlang] add PropEr (property-based testing tool) support #1102

merged 10 commits into from
Oct 27, 2018

Conversation

jfacorro
Copy link
Contributor

@jfacorro jfacorro commented Sep 25, 2018

Used to generate PropEr generators for property-based testing

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.3.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Creates an Erlang project that can be used to perform property-based testing using the PropEr library. The project includes a client where the request body is generated from the OpenAPI specification.

@jfacorro
Copy link
Contributor Author

Copying the technical comittee @tsloughter

@wing328 wing328 added this to the 3.3.0 milestone Sep 25, 2018
@tsloughter
Copy link
Contributor

This is great. Will take me a little bit to fully go over the patch but since it is just adding test code and not touching the generated logic it should be no problem.

Method = {{httpMethod}},
Host = application:get_env({{packageName}}, host, "http://localhost:8080"),
Path = ["{{{replacedPathName}}}"],
Body1 = {{^formParams.isEmpty}}{form, [{{#formParams}}{{#required}}{{^-first}}, {{/-first}}{<<"{{baseName}}">>, {{paramName}}}{{/required}}{{/formParams}}]++{{packageName}}_utils:optional_params([{{#formParams}}{{^required}}{{^-first}}, {{/-first}}'{{baseName}}'{{/required}}{{/formParams}}], _OptionalParams)}{{/formParams.isEmpty}}{{#formParams.isEmpty}}{{#bodyParams.isEmpty}}[]{{/bodyParams.isEmpty}}{{^bodyParams.isEmpty}}{{#bodyParams}}{{paramName}}{{/bodyParams}}{{/bodyParams.isEmpty}}{{/formParams.isEmpty}},
Copy link
Member

Choose a reason for hiding this comment

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

  1. Please use {{{baseName}}} instead of {{baseName}} to avoid special HTML characters being escaped.

  2. Instead of "Body1", what about naming it as "LocalVarHttpBody"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the comments.

  1. Fixed.
  2. Regarding your suggestion I would rather have a short name like Body, I think it's closer to the Erlang style, so I renamed Body1 to just Body since the number suffix didn't make any sense.

@@ -0,0 +1,543 @@
/*
* Copyright 2018 OpenAPI-Generator Contributors (https://openapi-generator.tech)
* Copyright 2018 SmartBear Software
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@tsloughter
Copy link
Contributor

I guess my only question is, shouldn't this be an option on the existing Erlang generator instead of a new one?

@wing328
Copy link
Member

wing328 commented Sep 29, 2018

@tsloughter very good question. I think depending on similarity. Personally, I prefer adding an option if the templates are similar with minor differences.

@wing328
Copy link
Member

wing328 commented Sep 29, 2018

When I ran rebar3 compile in samples/client/petstore/erlang-proper, I got the following errors:

➜  erlang-proper git:(jfacorro-erlang-proper) rebar3 compile
===> Fetching rebar3_proper ({pkg,<<"rebar3_proper">>,<<"0.11.1">>})
===> Downloaded package, caching at /Users/williamcheng/.cache/rebar3/hex/default/packages/rebar3_proper-0.11.1.tar
===> Compiling rebar3_proper
===> Verifying dependencies...
===> Fetching jsx ({pkg,<<"jsx">>,<<"2.9.0">>})
===> Version cached at /Users/williamcheng/.cache/rebar3/hex/default/packages/jsx-2.9.0.tar is up to date, reusing it
===> Fetching proper ({pkg,<<"proper">>,<<"1.3.0">>})
===> Downloaded package, caching at /Users/williamcheng/.cache/rebar3/hex/default/packages/proper-1.3.0.tar
===> Compiling proper
===> Compiling jsx
===> Compiling petstore
===> Compiling src/petstore_statem.erl failed
src/petstore_statem.erl:9: export_all flag enabled - all functions will be exported

@wing328 wing328 modified the milestones: 3.3.0, 3.3.1 Oct 1, 2018
@wing328 wing328 modified the milestones: 3.3.1, 3.3.2 Oct 15, 2018
@tsloughter
Copy link
Contributor

@jfacorro can you add the nowarn on export all? And can the statem go in a test or eqc directory?

@jfacorro
Copy link
Contributor Author

I guess my only question is, shouldn't this be an option on the existing Erlang generator instead of a new one?

@tsloughter I thought about this but decided against it. This is because the templates get unreadable very fast when there are several conditions involved. Having different generators also gives us the freedom to evolve them separately. The HTTP client part in the erlang-proper generator is a much simplified version of the erlang-client which seems to pull in some other dependencies.

When I ran rebar3 compile in samples/client/petstore/erlang-proper, I got the following errors:

@wing328 @tsloughter I added the nowarn_export_all so that it doesn't generate errors.

And can the statem go in a test or eqc directory?

@tsloughter Why do you think it would be better to put the statem in a test or proper directory? The whole generated Erlang application is for running and building the property-based testing model.

@wing328
Copy link
Member

wing328 commented Oct 24, 2018

@jfacorro thanks for the update.

@tsloughter please have another look

If no further question/feedback, I'll merge it on the coming weekend.

@wing328 wing328 merged commit fc0a0d2 into OpenAPITools:master Oct 27, 2018
@wing328 wing328 changed the title Generator erlang-proper [Erlang] add PropEr (property-based testing tool) support Oct 27, 2018
@wing328
Copy link
Member

wing328 commented Oct 27, 2018

@jfacorro PR merged into master. Thanks for your contribution.

I've sent out a tweet to proper the enhancement: https://twitter.com/oas_generator/status/1056109614494666752

@wing328
Copy link
Member

wing328 commented Oct 31, 2018

@jfacorro thanks for the PR, which is included in the v3.3.2 release: https://twitter.com/oas_generator/status/1057649626101112832

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
* Generator erlang-proper

Used to generate PropEr generators for property-based testing

* Fix binary/2 implementation. Add behaviour attribute.

* Remove line from copyright notice

* Avoid escpaing HTML and remove suffix from variable name

* Update samples

* Include querystring parameters

* We use export_all, don't consider warnings as errors

* List command sequence on failure

* Use hasConsumes instead

* Add nowarn_export_all, re-add warning_as_errors
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

3 participants