Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

Status: Active » Needs review
FileSize
2.94 KB

Also a pretty simple one. Will most likely need to remove the additional Tests directory from the new location for the file.

dawehner’s picture

We could throw out the "Unit" here, no need ... it will always be a unit test.

ParisLiakos’s picture

+++ b/core/modules/update/tests/Drupal/update/Tests/UpdateCoreUnitTest.phpundefined
@@ -31,7 +31,7 @@ public static function getInfo() {
+    require __DIR__ . '/../../../../update.fetch.inc';

this is a no:) we should convert this to a service/component instead, and then convert the test

also +1 @#2

dawehner’s picture

FileSize
11.69 KB

Then let's do it!

ParisLiakos’s picture

+++ b/core/modules/update/lib/Drupal/update/UpdateFetcher.phpundefined
@@ -0,0 +1,108 @@
+class UpdateFetcher {

needs a docblock

+++ b/core/modules/update/tests/Drupal/update/Tests/UpdateCoreUnitTest.phpundefined
@@ -2,43 +2,34 @@
+class UpdateCoreUnitTest extends UnitTestCase {

lets remove the Unit from the name

+++ b/core/modules/update/tests/Drupal/update/Tests/UpdateCoreUnitTest.phpundefined
@@ -2,43 +2,34 @@
+    //first test that we didn't break the trivial case.

should be
// First test...

Also should move to dataproviders:)

Status: Needs review » Needs work

The last submitted patch, drupal-2002116-4.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.24 KB
11.48 KB

Let's try and fix this, and incorporate that feedback.

I'm not sure about using a data provider here?

damiankloip’s picture

FileSize
4.93 KB
13.35 KB

Yeah, of course it is! :) Let's rock this out with a data provider.

The name also needs to go back to UpdateCoreUnitTest because we get a class declaration error.

dawehner’s picture

Awesome!!

+++ a/core/modules/update/tests/Drupal/update/Tests/UpdateCoreUnitTest.phpundefined
@@ -13,56 +13,82 @@
+   * Mimic this constant from bootstrap.inc.
+   */
+  const DRUPAL_CORE_COMPATIBILITY = '8.x';

Let's add a todo here ...

+++ a/core/modules/update/tests/Drupal/update/Tests/UpdateCoreUnitTest.phpundefined
@@ -13,56 +13,82 @@
+  protected function setUp() {
+    $config_factory = $this->getConfigFactoryStub(array('update.settings' => array('fetch_url' => 'http://www.example.com')));

What about a @inheritdoc here?

+++ a/core/modules/update/tests/Drupal/update/Tests/UpdateCoreUnitTest.phpundefined
@@ -13,56 +13,82 @@
+   * @dataProvider testUpdateBuildFetchUrlProvider

dataProvider all the things!

+++ a/core/modules/update/tests/Drupal/update/Tests/UpdateCoreUnitTest.phpundefined
@@ -13,56 +13,82 @@
+  public function testUpdateBuildFetchUrlProvider() {

Needs docs, sorry.

ParisLiakos’s picture

The name also needs to go back to UpdateCoreUnitTest because we get a class declaration error.

not sure i get why

jhedstrom’s picture

re: #10 I don't quite understand why either. This patch removes 'Unit' from the name and still passes w/o an error.

Status: Needs review » Needs work

The last submitted patch, update-module-phpunit-2002116-11.patch, failed testing.

ParisLiakos’s picture

also should probably be named UpdateFetcherTest since thats the class it tests

damiankloip’s picture

Status: Needs work » Needs review
FileSize
13.83 KB

Yeah, I think UpdateFetcherTest is a better name, Let's go with that.

RE #10, Did you see the test failures from the patch? These are the same as the ones in #7. So I'm guessing you just ran the single test from the CLI? Essentially that patch is the same as #7 which already failed for the same thing.

ParisLiakos’s picture

cannot redeclare the class...i forgot simpletest just loads everything it can find during tests.
Anyway UpdateFetcherTest to the rescue;)

i guess we need to fix #9 and then rtbc this:)

jhedstrom’s picture

I think this addresses the remaining issues in #9.

Status: Needs review » Needs work
Issue tags: -PHPUnit

The last submitted patch, update-module-phpunit-2002116-16.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: +PHPUnit
dawehner’s picture

Status: Needs review » Needs work

Just a few nitpicks.

+++ b/core/modules/update/tests/Drupal/update/Tests/UpdateFetcherTest.phpundefined
@@ -0,0 +1,101 @@
+   * Tests that buildFetchUrl() builds the URL correctly.
...
+  public function testUpdateBuildFetchUrl(array $project, $site_key, $expected) {

Let's add some docs.

+++ b/core/modules/update/tests/Drupal/update/Tests/UpdateFetcherTest.phpundefined
@@ -0,0 +1,101 @@
+  public function testUpdateBuildFetchUrlProvider() {

and rename the provider to providerTestUpdateBuildFetchUrl

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
14.6 KB
1.68 KB

This should address #19.

Status: Needs review » Needs work
Issue tags: -PHPUnit

The last submitted patch, update-module-phpunit-2002116-20.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +PHPUnit
dawehner’s picture

Status: Needs review » Needs work
index bfbd4a0..dbbfe2b 100644
--- a/core/modules/update/lib/Drupal/update/Tests/UpdateCoreTest.php

--- a/core/modules/update/lib/Drupal/update/Tests/UpdateCoreTest.php
+++ b/core/modules/update/lib/Drupal/update/Tests/UpdateCoreTest.phpundefined

@@ -2,7 +2,7 @@
+ * Definition of Drupal\update\Tests\UpdateFetcherTest.

This change is just out of scope, sorry.

+++ b/core/modules/update/lib/Drupal/update/UpdateFetcher.phpundefined
@@ -0,0 +1,111 @@
+ * Builds URL's to fetch module update information.
+ */
+class UpdateFetcher {

I guess on the longrun the UpdateFetcher will do more then just building URL's?

+++ b/core/modules/update/lib/Drupal/update/UpdateFetcher.phpundefined
@@ -0,0 +1,111 @@
+  public function buildFetchUrl($project, $site_key = '') {
...
+  public function getFetchBaseUrl($project) {

let's typehing project with array

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
7.34 KB
15.44 KB

i fixed points above and also moved anything related with Guzzle into the service, since thats what you expect it to do anyway

Status: Needs review » Needs work
Issue tags: -PHPUnit

The last submitted patch, update-module-phpunit-2002116-24.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +PHPUnit

The last submitted patch, update-module-phpunit-2002116-24.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
861 bytes
15.67 KB

sigh, missed a use statement

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/update/lib/Drupal/update/UpdateFetcher.phpundefined
@@ -0,0 +1,148 @@
+   * @var \Guzzle\Http\Client
...
+  protected $httpClient;
...
+   * @param \Guzzle\Http\Client $http_client
+   *   A Guzzle client object.
...
+  public function __construct(ConfigFactory $config_factory, Client $http_client) {

Let's use the ClientInterface

+++ b/core/modules/update/lib/Drupal/update/UpdateFetcher.phpundefined
@@ -0,0 +1,148 @@
+  public function fetchProjectData(array $project, $site_key = '') {
+    $url = $this->buildFetchUrl($project, $site_key);
+    $data = '';
+    try {
+      $data = $this->httpClient
+        ->get($url, array('Accept' => 'text/xml'))
+        ->send()
+        ->getBody(TRUE);
+    }
+    catch (RequestException $exception) {
+      watchdog_exception('update', $exception);
+    }
+    return $data;

I guess we should have a unit test for this method as well.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
1.24 KB
15.7 KB

I fixed the typehint.
i am not sure how to test this method, neither if i should..it has potential to cause failures under different enviroments, depending on the network settings

dawehner’s picture

Isn't the idea with guzzl that you can actually mock the http request?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Let's get it in then.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Converting this to an injected class is good, but making it a service seems a bit like overkill to me - it's really not intended to be used outside of update module.

ParisLiakos’s picture

it's really not intended to be used outside of update module.

well, the BookManager wont be used out of book module as well, but its a service since somehow we need to manage its dependencies. Same here.

ParisLiakos’s picture

dawehner’s picture

Well you can always just define the depenencies in the controller you need for the custom class and inject it then.

Mile23’s picture

Status: Needs review » Needs work

Needs a re-roll.

$ git apply update-module-phpunit-2002116-30.patch 
error: patch failed: core/modules/update/update.fetch.inc:274
error: core/modules/update/update.fetch.inc: patch does not apply
damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.27 KB
16.3 KB

rerolled (quickly and it's early) and made UpdateFetcher not a service.

Status: Needs review » Needs work

The last submitted patch, 2002116-38.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
16.3 KB

Meh

Status: Needs review » Needs work

The last submitted patch, 2002116-40.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
794 bytes
16.3 KB

Constant needs to switch to \Drupal.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/update/tests/Drupal/update/Tests/UpdateFetcherTest.php
@@ -0,0 +1,114 @@
+if (!defined('DRUPAL_CORE_COMPATIBILITY')) {
+  define('DRUPAL_CORE_COMPATIBILITY', '8.x');
+}
+

Is there a follow up to remove this constant in update module itself?

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

jhedstrom’s picture