Task to convert core/modules/update/lib/Drupal/update/Tests/UpdateCoreUnitTest.php to phpunit.

See #1938068: [Meta] Convert UnitTestBase to PHPUnit.

Files: 
CommentFileSizeAuthor
#42 2002116-42.patch16.3 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 59,040 pass(es).
[ View ]
#42 interdiff-2002116-42.txt794 bytesdamiankloip
#40 2002116-40.patch16.3 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 58,895 pass(es), 0 fail(s), and 93 exception(s).
[ View ]
#38 2002116-38.patch16.3 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/update/update.fetch.inc.
[ View ]
#38 interdiff-2002116-38.txt1.27 KBdamiankloip
#30 update-module-phpunit-2002116-30.patch15.7 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 58,082 pass(es).
[ View ]
#30 interdiff.txt1.24 KBParisLiakos
#28 update-module-phpunit-2002116-27.patch15.67 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 56,758 pass(es).
[ View ]
#28 interdiff.txt861 bytesParisLiakos
#24 update-module-phpunit-2002116-24.patch15.44 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 58,013 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#24 interdiff.txt7.34 KBParisLiakos
#20 interdiff.txt1.68 KBjhedstrom
#20 update-module-phpunit-2002116-20.patch14.6 KBjhedstrom
PASSED: [[SimpleTest]]: [MySQL] 58,091 pass(es).
[ View ]
#16 interdiff.txt1.14 KBjhedstrom
#16 update-module-phpunit-2002116-16.patch13.97 KBjhedstrom
PASSED: [[SimpleTest]]: [MySQL] 56,498 pass(es).
[ View ]
#14 2002116-14.patch13.83 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,837 pass(es).
[ View ]
#11 update-module-phpunit-2002116-11.patch13.33 KBjhedstrom
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#11 interdiff.txt913 bytesjhedstrom
#8 2002116-8.patch13.35 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,324 pass(es).
[ View ]
#8 interdiff-2002116-8.txt4.93 KBdamiankloip
#7 2002116-7.patch11.48 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#7 interdiff-2002116-7.txt1.24 KBdamiankloip
#4 drupal-2002116-4.patch11.69 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,396 pass(es), 70 fail(s), and 6 exception(s).
[ View ]
#1 convert-update-module-phpunit-2002116-01.patch2.94 KBjhedstrom
PASSED: [[SimpleTest]]: [MySQL] 55,862 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new2.94 KB
PASSED: [[SimpleTest]]: [MySQL] 55,862 pass(es).
[ View ]

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

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

+++ 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

StatusFileSize
new11.69 KB
FAILED: [[SimpleTest]]: [MySQL] 55,396 pass(es), 70 fail(s), and 6 exception(s).
[ View ]

Then let's do it!

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new1.24 KB
new11.48 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

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

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

StatusFileSize
new4.93 KB
new13.35 KB
PASSED: [[SimpleTest]]: [MySQL] 55,324 pass(es).
[ View ]

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.

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.

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

not sure i get why

StatusFileSize
new913 bytes
new13.33 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new13.83 KB
PASSED: [[SimpleTest]]: [MySQL] 55,837 pass(es).
[ View ]

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.

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:)

StatusFileSize
new13.97 KB
PASSED: [[SimpleTest]]: [MySQL] 56,498 pass(es).
[ View ]
new1.14 KB

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.

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

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

Status:Needs work» Needs review
StatusFileSize
new14.6 KB
PASSED: [[SimpleTest]]: [MySQL] 58,091 pass(es).
[ View ]
new1.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.

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

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

Status:Needs work» Needs review
StatusFileSize
new7.34 KB
new15.44 KB
FAILED: [[SimpleTest]]: [MySQL] 58,013 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new861 bytes
new15.67 KB
PASSED: [[SimpleTest]]: [MySQL] 56,758 pass(es).
[ View ]

sigh, missed a use statement

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.

Status:Needs work» Needs review
StatusFileSize
new1.24 KB
new15.7 KB
PASSED: [[SimpleTest]]: [MySQL] 58,082 pass(es).
[ View ]

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

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

Status:Needs review» Reviewed & tested by the community

Let's get it in then.

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.

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.

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

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

Status:Needs work» Needs review
StatusFileSize
new1.27 KB
new16.3 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/update/update.fetch.inc.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new16.3 KB
FAILED: [[SimpleTest]]: [MySQL] 58,895 pass(es), 0 fail(s), and 93 exception(s).
[ View ]

Meh

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new794 bytes
new16.3 KB
PASSED: [[SimpleTest]]: [MySQL] 59,040 pass(es).
[ View ]

Constant needs to switch to \Drupal.

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?

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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