DRUP-747 SDKConnector refactoring, mock API responses in tests and extra test for Authentication form #186

mxr576 · 2019-04-30T13:24:48Z

For DRUP-747 we have to be able to replace the API client returned by the SDKConnector in a test. This could be possible without refactoring it but it is better for everyone if we do a last refactoring on the SDKConnector service before the stable release.

I wanted to do this refactoring for a long time ago this seems the best time to do that. These changes simplify the logic, apply OOP- and service decorator best practices on the SDKConnector. Because 99% of code relies on the SdkConnector::getClient() method this is a smaller BC breaking change.

Check the commit message for more details.

Latest Travis build: https://travis-ci.org/mxr576/apigee-devportal-drupal/builds/535298059

mxr576 · 2019-05-03T07:45:31Z

Figuring out the best way for mocking and implementing it takes more time. Let's get this merged before the next release and I'll continue with the mocking peace in the next sprint.

mxr576 · 2019-05-03T09:09:52Z

https://travis-ci.org/mxr576/apigee-devportal-drupal/builds/526461065

mxr576 · 2019-05-03T09:54:24Z

The test has only failed because of this: 3541a43

Jaesin · 2019-05-04T06:17:54Z

modules/apigee_edge_debug/src/SDKConnector.php


 /**
 * Service decorator for SDKConnector.
 */
-class SDKConnector extends OriginalSDKConnector implements SDKConnectorInterface {
+final class SDKConnector implements SDKConnectorInterface {


This will force more re-work in the monetization module. Many classes in this module are being marked as final which keeps them from being extended.

I checked the Monetization module before I created this PR and I haven't found anything that this change would affect or make it impossible to rewrite with encapsulation. (Which is a better design in this case, because we are not trying to build an API around a service which just builds the API client that other services are using.)
@Jaesin I may have missed something, could you give me a concreate example that you see problematic?

Double check it, I used what it is currently in the 8.x-1.x branch. The MINT module only depends on the SDKConnectorInterface and the method that it provides, it does not depend on a concrete implementation in the SDKConnector. 🎉 The MINT module does not use \Drupal\apigee_edge\SDKConnectorInterface::buildClient() method which has been removed.

@Jaesin let me know if I missed something.

mxr576 · 2019-05-07T16:29:33Z

It would be great if this could be in the next release.

Jaesin · 2019-05-07T21:28:27Z

After rebasing onto 8.x-1.x and testing with Drupal 8.7.0, I consistently get errors:

mxr576 · 2019-05-08T06:13:32Z

The reason for the failed test: https://travis-ci.org/mxr576/apigee-devportal-drupal/builds/529631971

https://travis-ci.org/mxr576/apigee-devportal-drupal/builds/529631971

mxr576 · 2019-05-09T07:18:25Z

@Jaesin PR updated, it is almost impossible to make a build to pass on Travis thanks for drupal-composer/drupal-scaffold#105

Here is a proof from my local that Authentication form test now passes:

Runtime:       PHP 7.2.15 with Xdebug 2.6.1
Configuration: /var/www/html/web/core/phpunit.xml.dist

Testing Drupal\Tests\apigee_edge\FunctionalJavascript\Form\AuthenticationFormJsTest
Test 'Drupal\Tests\apigee_edge\FunctionalJavascript\Form\AuthenticationFormJsTest::testAuthenticationForm' started
Test 'Drupal\Tests\apigee_edge\FunctionalJavascript\Form\AuthenticationFormJsTest::testAuthenticationForm' ended
Test 'Drupal\Tests\apigee_edge\FunctionalJavascript\Form\AuthenticationFormJsTest::testKeyAddForm' started
Test 'Drupal\Tests\apigee_edge\FunctionalJavascript\Form\AuthenticationFormJsTest::testKeyAddForm' ended


Time: 2.31 minutes, Memory: 6.00MB

OK (2 tests, 48 assertions)

mxr576 · 2019-05-09T13:32:01Z

Finally: https://travis-ci.org/mxr576/apigee-devportal-drupal/builds/530265789

mxr576 · 2019-05-13T08:12:02Z

@Jaesin Could you finalize the review of this PR, please? My upcoming commits depends on these changes.

mxr576 · 2019-05-14T09:41:43Z

Okay, as I haven't got an approval on this PR yet I pushed new commits. Please review.

mxr576 · 2019-05-14T13:38:20Z

src/SDKConnector.php

+    // We do this because otherwise SDK connector service decorators that
+    // decorates the getClient() method would not be called.
+    if ($key === NULL) {
+      $client = $this->container->get('apigee_edge.sdk_connector')->getClient();


Well, this is probably a clear sign that this method should not belong to the SDK connector (as a service and interface) because it depends on the current instance of the SDK connector service. So if I could rewrite this completely I would decouple this from the sdk connector. The SDK connector would only build the client, nothing more. Probably it should be renamed to ClientBuilder. It would depend on a new service that could return the saved the Apigee Edge authentication credentials (username/pwd/endpoint/org) from whichever storage the current service uses (by default the Key) and the testConnection() method would be implemented in a separate service as weéé that would depend on the SDK connector (client builder) and the authentication credentials provider service.
But I do not have to think we have a capacity for this level of rewrite.

mxr576 · 2019-05-16T09:16:11Z

@Jaesin could you take a look at this PR again?

mxr576 · 2019-05-17T06:01:41Z

Rebased PR from upstream changes.

googlebot · 2019-05-20T06:15:08Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-21T09:20:43Z

☹️ Sorry, but only Googlers may change the label cla: yes.

mxr576 · 2019-05-21T13:40:42Z

https://travis-ci.org/mxr576/apigee-devportal-drupal/builds/535298059

mxr576 · 2019-05-22T05:35:05Z

@cnovak please verify the CLA on this PR.
@Jaesin please finalize the review, this should be in RC5 as we discussed.

googlebot · 2019-05-22T22:44:58Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-23T00:04:43Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Jaesin · 2019-05-23T01:03:45Z

I re-based the PR branch to separate out code comment changes.

I created another PR for Logic fixes from this PR here: #200

There are three other types of changes here.

Refactoring.
A new API Response mocking system.
Additional tests.

The easiest way to review this PR is commit by commit.

I don't see the refactoring as necessary to achieve the objective of additional testing of the authentication form but it seems that being able to mock API responses is. There is already an outdated PR (#79) to provide such functionality. If we are working on a solution for this, shouldn't we follow up on that PR and provide a system that will be compatible or work with what's in the apigee_m10n module?

* Made sure the SDKConnector is open for extension but closed for modification. * As a consequence, the SDKConnector became final. * Merged the buildClient() method with the getClient() method and removed from the interface. * Added missing return types and concretized type of thrown exceptions. * Removed dead-code and unused properties.

The new test case covers: apigee#179 (review)

apigee/apigee-client-php#54

DRUP-241

mxr576 · 2019-05-23T06:12:09Z

Rebased the PR from master: https://travis-ci.org/mxr576/apigee-devportal-drupal/builds/536110705

Indeed, this PR contained multiple, kinda unrelated changes and I did not like that either, but I could not merge the smaller pieces without approval in the previous weeks.

Originally I thought the SDKConnector refactoring is also required for proper mocking. When I actually started to work on the mocking I saw that there is the other way around. Although I would like to still close the SdkConnector class before the first stable gets released because it should not be considered as an API by developers. It is not an abstract class, it is a concrete implementation that uses Key as storage. If someone needs something else, s/he should not subclass it from SdkConnector, s/he should build a new implementation by implementing the SdkConnectorInterface. This is KISS. I see very for reasons why someone would decouple this service from the module (mostly covered by the debug and the test module :) ), which is another reason why it should be closed.

Regarding mock testing, before I started to build this mocking "framework" I checked #79 and I tried to build something that ensures #79 could be easily migrated to the new implementation. What you have built there was a good for a start but it was also too concrete. I was not extensiable enough
This event-based approach is more robust. As you can see from this PR, you can implement any kind of API request subscriber. You can register multiple subscribers and they can decide whether they react to a request or not. Subscribers can work together for subsequent requests within a page call, for example, one captures the first API request, transforms it, saves it to shared storage and the next subscriber for the next request returns it as a response for the next API call. (In case of Apigee Edge, sometimes the API response payload for a CREATE is not the same what you get back for a GET for the same entity.) The possibilities are endless.
If this PR gets merged it does not mean that you immediately need to switch to this in the MINT module. The two implementations could live together.

mxr576 · 2019-05-29T09:59:26Z

@cnovak @Jaesin So is there a chance to get this in to the stable version? It should have been released in RC5, but communication has stuck again and it has not been added... Friendly reminder that this PR also contains a fix which blocks the next release of the PHP API client.
Should I move that to a new PR?

mxr576 · 2019-06-17T06:48:29Z

#224 has just arrived, it would be great if this PR could be merged before somebody starts extending the old SDKConnector implementation in its custom module.

cnovak · 2019-10-01T18:14:31Z

Closing this ticket due to new strategy for testing

mxr576 requested review from Jaesin, cnovak and dawehner April 30, 2019 13:25

mxr576 added apigee_edge bc breaking BC breaking changes. labels Apr 30, 2019

mxr576 force-pushed the DRUP-747 branch from 01ad904 to 2467c19 Compare April 30, 2019 14:20

mxr576 changed the title ~~DRUP-747 Extra test coverage for Authentication form and SDKConnector refactoring~~ DRUP-747 SDKConnector refactoring May 3, 2019

mxr576 marked this pull request as ready for review May 3, 2019 07:45

Jaesin reviewed May 4, 2019

View reviewed changes

mxr576 assigned Jaesin May 7, 2019

mxr576 force-pushed the DRUP-747 branch from 2467c19 to 930b0bb Compare May 8, 2019 06:12

googlebot added the cla: yes Indicates CLA has been signed label May 9, 2019

mxr576 changed the title ~~DRUP-747 SDKConnector refactoring~~ DRUP-747 SDKConnector refactoring, mock API responses in tests and extra test for Authentication form May 14, 2019

mxr576 requested a review from Jaesin May 14, 2019 09:41

mxr576 commented May 14, 2019

View reviewed changes

mxr576 force-pushed the DRUP-747 branch from def9346 to 69051ad Compare May 17, 2019 06:00

googlebot removed the cla: yes Indicates CLA has been signed label May 20, 2019

mxr576 force-pushed the DRUP-747 branch from 5e13be5 to f74bc98 Compare May 21, 2019 09:17

mxr576 added cla: yes Indicates CLA has been signed and removed cla: no Indicates CLA has not been signed labels May 21, 2019

googlebot removed the cla: yes Indicates CLA has been signed label May 21, 2019

mxr576 force-pushed the DRUP-747 branch from f74bc98 to a3e1647 Compare May 21, 2019 13:30

googlebot added the cla: no Indicates CLA has not been signed label May 21, 2019

cnovak added cla: yes Indicates CLA has been signed and removed cla: no Indicates CLA has not been signed labels May 22, 2019

Jaesin force-pushed the DRUP-747 branch from a3e1647 to b6e53eb Compare May 23, 2019 00:04

Jaesin mentioned this pull request May 23, 2019

DRUP-789 Fix PHP notice caused by missing array key #200

Merged

mxr576 added 7 commits May 23, 2019 07:33

Make sure that service decorators get called

2da5a1a

Allow API responses to be mocked in tests

4c64e09

Added extra test coverage to Authentication form

ed33c83

The new test case covers: apigee#179 (review)

Small code optimizations

8255ae1

Fix test after apigee/apigee-client-php/pull/54

9ca7078

apigee/apigee-client-php#54

Run tests with the latest dev version of the PHP API Client

abfb59c

DRUP-241

mxr576 force-pushed the DRUP-747 branch from b6e53eb to abfb59c Compare May 23, 2019 05:47

mxr576 mentioned this pull request Jun 13, 2019

[DRUP-732] RBAC on API Docs apigee/apigee-api-catalog-drupal#26

Closed

cnovak closed this Oct 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRUP-747 SDKConnector refactoring, mock API responses in tests and extra test for Authentication form #186

DRUP-747 SDKConnector refactoring, mock API responses in tests and extra test for Authentication form #186

mxr576 commented Apr 30, 2019 •

edited

Loading

mxr576 commented May 3, 2019

mxr576 commented May 3, 2019

mxr576 commented May 3, 2019

Jaesin May 4, 2019

mxr576 May 6, 2019 •

edited

Loading

mxr576 May 6, 2019

mxr576 commented May 7, 2019

Jaesin commented May 7, 2019

mxr576 commented May 8, 2019 •

edited

Loading

mxr576 commented May 9, 2019

mxr576 commented May 9, 2019

mxr576 commented May 13, 2019

mxr576 commented May 14, 2019

mxr576 May 14, 2019 •

edited

Loading

mxr576 commented May 16, 2019

mxr576 commented May 17, 2019

googlebot commented May 20, 2019

googlebot commented May 21, 2019

mxr576 commented May 21, 2019

mxr576 commented May 22, 2019

googlebot commented May 22, 2019

googlebot commented May 23, 2019

Jaesin commented May 23, 2019

mxr576 commented May 23, 2019

mxr576 commented May 29, 2019 •

edited

Loading

mxr576 commented Jun 17, 2019

cnovak commented Oct 1, 2019

DRUP-747 SDKConnector refactoring, mock API responses in tests and extra test for Authentication form #186

DRUP-747 SDKConnector refactoring, mock API responses in tests and extra test for Authentication form #186

Conversation

mxr576 commented Apr 30, 2019 • edited Loading

mxr576 commented May 3, 2019

mxr576 commented May 3, 2019

mxr576 commented May 3, 2019

Jaesin May 4, 2019

Choose a reason for hiding this comment

mxr576 May 6, 2019 • edited Loading

Choose a reason for hiding this comment

mxr576 May 6, 2019

Choose a reason for hiding this comment

mxr576 commented May 7, 2019

Jaesin commented May 7, 2019

mxr576 commented May 8, 2019 • edited Loading

mxr576 commented May 9, 2019

mxr576 commented May 9, 2019

mxr576 commented May 13, 2019

mxr576 commented May 14, 2019

mxr576 May 14, 2019 • edited Loading

Choose a reason for hiding this comment

mxr576 commented May 16, 2019

mxr576 commented May 17, 2019

googlebot commented May 20, 2019

googlebot commented May 21, 2019

mxr576 commented May 21, 2019

mxr576 commented May 22, 2019

googlebot commented May 22, 2019

googlebot commented May 23, 2019

Jaesin commented May 23, 2019

mxr576 commented May 23, 2019

mxr576 commented May 29, 2019 • edited Loading

mxr576 commented Jun 17, 2019

cnovak commented Oct 1, 2019

mxr576 commented Apr 30, 2019 •

edited

Loading

mxr576 May 6, 2019 •

edited

Loading

mxr576 commented May 8, 2019 •

edited

Loading

mxr576 May 14, 2019 •

edited

Loading

mxr576 commented May 29, 2019 •

edited

Loading