Task to convert the json unit tests in the system module to use phpunit. Also will require deprecating drupal_json_encode/decode, and making a json component.

See #1938068: Convert UnitTestBase to PHPUnit.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

Status: Active » Needs review
FileSize
6.68 KB

Patch moves logic from drupal_json_encode/decode() to a utility component, and moves the tests to phpunit.

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

The last submitted patch, system-json-2016299-01.patch, failed testing.

Berdir’s picture

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

#1: system-json-2016299-01.patch queued for re-testing.

ParisLiakos’s picture

not sure if its worthy to convert those, since they ll probably die in favor of JsonResponse

jhedstrom’s picture

I'd still like to see them converted and then removed, rather than left in place should the JsonResponse work not happen.

ParisLiakos’s picture

Sure, fine by me!

+++ b/core/lib/Drupal/Component/Utility/Json.phpundefined
@@ -0,0 +1,37 @@
+class Json {
+  /**

needs a newline in between

+++ b/core/lib/Drupal/Component/Utility/Json.phpundefined
@@ -0,0 +1,37 @@
+  public static function encode($var) {
...
+  public static function decode($var) {

@param docs needed

+++ b/core/tests/Drupal/Tests/Component/Utility/JsonTest.phpundefined
@@ -2,21 +2,22 @@
+ * Definition of Drupal\Tests\Component\Utility\JsonTest.

Contains \Drupal\....

About the test: Maybe you could break it up to methods using dataProviders so we make it a bit more readable than now?

dawehner’s picture

FileSize
10.56 KB
7.09 KB

I am not convinced that data providers would improve the readability, but splitting it up would certainly help.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

breaking it up that way works too:)
thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs a reroll

curl https://drupal.org/files/drupal-2016299-7.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 10810  100 10810    0     0   9141      0  0:00:01  0:00:01 --:--:-- 11041
error: patch failed: core/includes/common.inc:1
error: core/includes/common.inc: patch does not apply
dawehner’s picture

Status: Needs work » Needs review
FileSize
11.31 KB

Just another rerole.

ParisLiakos’s picture

+++ b/sites/default/default.settings.phpundefined
@@ -289,9 +289,6 @@
- * - Note that this debugging markup will cause automated tests that directly
- *   check rendered HTML to fail. When running automated tests, 'twig_debug'
- *   should be set to FALSE.

irrelevant hunk?

dawehner’s picture

FileSize
10.56 KB

Good catch!

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

awesome thanks!

Status: Reviewed & tested by the community » Needs work
Issue tags: -PHPUnit

The last submitted patch, drupal-2016299-11.patch, failed testing.

ParisLiakos’s picture

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

#12: drupal-2016299-11.patch queued for re-testing.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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

jhedstrom’s picture