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

Integration tests #75

Merged
merged 10 commits into from
Dec 12, 2018
Merged

Integration tests #75

merged 10 commits into from
Dec 12, 2018

Conversation

eguzki
Copy link
Member

@eguzki eguzki commented Nov 27, 2018

  • Copy service integration tests
  • Update service integration tests (with and without --rules-only flag
  • Remote add integration tests
  • Remote rename integration tests
  • Remote delete integration tests
  • integration tests documentation (secrets)

@eguzki eguzki requested a review from mikz November 27, 2018 11:37
require 'webmock/rspec'
WebMock.disable_net_connect!
WebMock.allow_net_connect!
Copy link
Contributor

Choose a reason for hiding this comment

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

There definitely should not be need for enabling net connect for all the tests.

And even for the integration, I think it makes sense to stub those API calls.

And if we think that it is that important to have a real connection test, then at least use VCR and schedule casette refresh.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is more like philosophical discussion.

Should integration tests do real integration with 3rd party API's (3scale API in this case)???

IMO depends on many aspects about the project context. Real integration tests are harder to implement, maintain and long way slower tests. We have to deal with secrets and prepare and clean everything before/after (to be implemented yet) to keep the environment as clean as possible.

The pro is real integration tests. Do we need this? I am inclined to think that we do. A change in 3scale API that breaks toolbox functionality will be "immediately" detected by CI integration environment that runs periodically the integration tests (nightly??).

If you think it is not needed, easier life for everybody and I will stub API calls, specifically the calls to 3scale-api-ruby client

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a philosophical discussion.
You can't have enabled net access for the whole test suite. If you need it, then just for some part.

In the testing pyramid, you want as many low-level tests as you can have and add less and less high-level tests.

The underlying client already has integration tests: https://github.com/3scale/3scale-api-ruby/blob/master/spec/integration/account_spec.rb

Do you need to repeat the same tests here?
Also, to be a proper integration test suite, it should clean up after itself. The 3scale-api-ruby does not really do that fully and that is a pity as it will grow over time.

I'm not arguing we don't need integration tests. I'm arguing running against the internet can't be the only way to verify this works. I think that is definitely a nice thing to do.

You need some verification that the integration works or is supposed to work without access to the internet. You either need to run the test suite twice, once with mocks and second time connecting to the internet, so there is at least some verification, or use VCR and record the interactions. Running the test suite only against the internet is just a terrible idea.

Another way is to provide the "mock" mode directly in the 3scale-api-ruby library.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I will mock 3scale-api-ruby

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea behind the injectable HTTP client was that it could be faked, so that is the way to go IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Low level transport is not injected either. ThreeScale::API::HttpClient is injected, which is a wrapper around Net::HTTP client, which is implementation detail.

By mocking 3scale api library we do test the use of ThreeScale::API::Client, because all method calls, passed args and responses are under control. That way, interface is tested. We just do not need to run ThreeScale::API::Client code. I do not see advantage mocking ThreeScale::API::HttpClient but not ThreeScale::API::Client.

If we want to mock, either we mock ThreeScale::API::Client or stub http calls with webmock. IMO it is the way to go.

Copy link
Contributor

@mikz mikz Nov 28, 2018

Choose a reason for hiding this comment

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

Using a fake ThreeScale::API::HttpClient in ThreeScale::API::Client allows you to implement fake, mocked request/response cycle, that remembers state. So for example, if you create a Service you'd be able to return it back when requested.
That is, of course, possible with webmock too, but a bit more annoying and less reusable.

3scale-api-ruby providing a fake implementation, that is verified against real API, so other projects using it don't have to think about it is a pretty good benefit.

edit: Providing an alternative implementation of the http client opens doors to many other benefits, like the possibility to have "fixtures" based responses based on some folder structure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented integration tests with internet access do the following:

  • Run command (let's say service copy command)
  • Use 3scale api to request new service components and check responses against expected information. Checking settings, methods, metrics, application plans, limits and mapping rules have been created and with expected attributes.

Integration tests with mocked HttpClient would do the following:

  • Run command (let's say service copy command)
  • Check expected HttpClient api calls are done with expected params
  • Check toolbox return code when HttpClient api calls response is 500, 403 or unexpected response codes.

The former integration tests (already implemented) perform essentially test integration between 3Scale API and the toolbox. While the latter integration tests perform essentially test integration between toolbox inner components.

IMO testing integration between 3scale API and the toolbox is necessary. This toolbox is not generic http client. The toolbox has logic for high level tasks on 3scale API, not in any other API. Thus, checking integration with 3scale API seems of great benefit. It provides harness and confidence about working commands. Imagine some logic at 3scale API platform level that controls Service components. For instance, redirect_url attribute of mapping rules is not copied unless Service deployment_option is set to be self-managed. This kind of logic affecting final result would not be detected without testing integration with 3Scale API.

Underlying client integration tests with net access do not cover high level integration like exposed above.

So, I will add integration tests with mocked HttpClient, but I propose to keep integration with 3Scale API with internet access. Besides, we can enable net connect for a given set of integration tests, no problem about that.

WDYT?

Copy link
Contributor

@mikz mikz Nov 29, 2018

Choose a reason for hiding this comment

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

I'll be repeating myself and probably have nothing more to add, but I'll try again.

The tests for with/without internet access can be the same tests. It is just about the transport layer HttpClient talking to a remote server or different object with the same interface talking to local memory.

I'm saying, that internet only tests suck because you can't develop without internet, on a plane, when there is a power outage, ... For that reason, the same tests should have some fallback, like a stubbed HTTP client or something like VCR.

If done properly, you can test that the attributes specific to some flags passed to the toolbox are being passed upstream or not. You can totally verify that the redirect_url is passed when the deployment_option is self-managed even with the fake http client talking to local memory or using VCR.

For testing that redirect_url is passed when you have some value of deployment_option you need a unit test, not an integration one. And it definitely does not need to talk to internet to verify that.

I don't suggest to have two kinds of integration tests: remote and stubbed. I'm only requesting, that there is a way to run the integration tests without internet too. That can be achieved by injecting a custom http client that does not talk to internet and just stored requests / responses from local memory to fake 3scale APIs or using VCR.

And yes, net connect should be allowed only for some part of the test suite, not for the whole.

Ultimately this is your call and I'm not sure if I have anything more to add to the discussion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Easily put, the basic idea is to include the logic running on 3scale API in the tests. If iether toolbox logic or 3scale API logic changes, the tests will notify.

@eguzki eguzki changed the title (WIP) Integration tests Integration tests Dec 5, 2018
@eguzki
Copy link
Member Author

eguzki commented Dec 5, 2018

Took quite a big effort to implement, but here we go with smart integration tests.

Integration tests can be run locally or against a real 3scale account. When details of the account are set via environment variables, integration tests are run against given account. Otherwise, tests are run locally with mocked 3scale clients.

Ready to be reviewed

@eguzki
Copy link
Member Author

eguzki commented Dec 12, 2018

rebased to resolve conflicts

@eguzki eguzki merged commit ab990a5 into master Dec 12, 2018
@eguzki eguzki deleted the integration-tests branch December 12, 2018 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants