Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[cpprest] Make the generated APIs mockable. #9

Merged
merged 3 commits into from
Apr 17, 2018

Conversation

bjoernbaeumler
Copy link

We want to able to unit test our code that uses the generated API code.
In order to do that we need a virtual interface to mock away the
generated API code.

This PR introduces two new config options for the cpprest generator:

  • "generateInterfacesForApis" will generate an abstract base class
    (interface) for all APIs.
  • "generateGMocksForApis" will additionally generate Google Mock classes
    for the APIs. This config option of course implies the first one.

If the options are not set only minor white space changes will result
(see the changes in the petstore example).

We want to able to unit test our code that uses the generated API code.
In order to do that we need a virtual interface to mock away the
generated API code.

This PR introduces two new config options for the cpprest generator:
* "generateInterfacesForApis" will generate an abstract base class
  (interface) for all APIs.
* "generateGMocksForApis" will additionally generate Google Mock classes
  for the APIs. This config option of course implies the first one.

If the options are not set only minor white space changes will result
(see the changes in the petstore example).
@@ -110,6 +112,10 @@ public CppRestClientCodegen() {
addOption(DEFAULT_INCLUDE,
"The default include statement that should be placed in all headers for including things like the declspec (convention: #include \"Commons.h\" ",
this.defaultInclude);
addOption(GENERATE_INTERFACES_FOR_APIS,
"Generate abstract base classes (interfaces) for APIs. This allows to mocks APIs (e.g. for unit testing).");
Copy link

Choose a reason for hiding this comment

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

Should be This allows to **mock** APIs

Copy link
Author

Choose a reason for hiding this comment

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

Yep, will fix it.

@@ -110,6 +112,10 @@ public CppRestClientCodegen() {
addOption(DEFAULT_INCLUDE,
"The default include statement that should be placed in all headers for including things like the declspec (convention: #include \"Commons.h\" ",
this.defaultInclude);
addOption(GENERATE_INTERFACES_FOR_APIS,
"Generate abstract base classes (interfaces) for APIs. This allows to mocks APIs (e.g. for unit testing).");
Copy link

Choose a reason for hiding this comment

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

I wonder if we should simply force this, and not make it an option.
I agree that the gmock stuff should be optional because we don't want to force users into gmock.
But having interfaces shouldn't impact anyone already using the tool, and "forcing" them on everyone will help make the overall project more maintainable (e.g. if one day the sample code is tested, should there be tests for both versions?).

Copy link
Author

Choose a reason for hiding this comment

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

My reasoning was that having the interfaces will increase compile and runtime costs (e.g. an additional virtual function call). So if you don't need them, why would you want to pay for them? On the other hand, compared to a HTTP request, a virtual function call is negligible so this argument is not really valid.
Your argument regarding maintainability is certainly right and probably the stronger one, especially for an open source project. So I will remove this config option.

@@ -35,7 +35,8 @@ namespace api {

using namespace io::swagger::client::model;

class StoreApi

Choose a reason for hiding this comment

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

I think we should avoid spurious whitespace changes in generated code as it's just diff noise.

Copy link
Author

Choose a reason for hiding this comment

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

Now that we always generate the ABCs it's no longer an issue.

@bjoernbaeumler bjoernbaeumler requested a review from c-bebop April 17, 2018 13:34
@bjoernbaeumler bjoernbaeumler merged commit d9937b2 into master Apr 17, 2018
Peaches491 added a commit to zooxco/openapi-generator that referenced this pull request Jul 18, 2018
Peaches491 added a commit to zooxco/openapi-generator that referenced this pull request Jul 18, 2018
wing328 pushed a commit to OpenAPITools/openapi-generator that referenced this pull request Jul 24, 2018
* Port GMock feature from NativeInstruments

swagger-codegen fork:
NativeInstruments/swagger-codegen#9

* Update petstore for Mockable APIs

* Fix shared_ptr in templates for File params

* Add guards in templates for GMock APIs

* Regenerate samples without GMocks

* Add useful constructors for GMock APIs

* Add constructors to API header interface

* Update samples with explicit monadic constructors

* Add default implementations for destructors
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
* Port GMock feature from NativeInstruments

swagger-codegen fork:
NativeInstruments/swagger-codegen#9

* Update petstore for Mockable APIs

* Fix shared_ptr in templates for File params

* Add guards in templates for GMock APIs

* Regenerate samples without GMocks

* Add useful constructors for GMock APIs

* Add constructors to API header interface

* Update samples with explicit monadic constructors

* Add default implementations for destructors
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants