TwigExtensionTest::testFormatDate() doesn't make much sense

plz review

Hi anantjain60.

Berdir points two problems, we can review them in order:

That test uses its own mock date formatter (with wrong arguments), it could just as well use the actual date string that it expects.

If you check: Drupal\Core\Datetime::format() method signature you will see:

public function format($timestamp, $type = 'medium', $format = '', $timezone = NULL, $langcode = NULL) {
...
}

That means that the first argument is a timestamp and the second the type if you check how the mock is being used you will see:

$this->assertEquals($this->dateFormatter->format('html_date'), $result);

The format method is receiving just the type and it should be receiving the timestamp first, so for this example, you need to define that variable (in your patch you don't even define the $timestamp variable you just use it.)

And the other problem is:

Also strange that time doesn't seem to be defined at all, would make more sense to explicitly define that and also explicitly assert the received arguments in the mocked method.

If you see what is being rendered:

$result = $twig->render('{{ time|format_date("html_date") }}');

The time variable is not defined, so you need to defined it as well,

If you check the render signature:

/**
     * Renders a template.
     *
     * @param string $name    The template name
     * @param array  $context An array of parameters to pass to the template
     *
     * @return string The rendered template
     *
     * @throws Twig_Error_Loader  When the template cannot be found
     * @throws Twig_Error_Syntax  When an error occurred during compilation
     * @throws Twig_Error_Runtime When an error occurred during rendering
     */
    public function render($name, array $context = array())

You can pass a $context to the render, so you can define the "time" variable that is going to be rendered.

What I suggest is:

Define the $timestamp variable with a fixed value.

$timestamp = strtotime('1978-11-19');

Pass that variable to the render

  $twig->render('{{ time|format_date("html_date") }}', ['time' => $timestamp]);

Pass that variable as well to the dateFormatter->format() Call

$this->assertEquals($this->dateFormatter->format($timestamp, 'html_date'), $result);

AND finally, edit the mock

$this->dateFormatter->expects($this->exactly(2))
      ->method('format')
      ->willReturn('1978-11-19');

To instead to return a fixed string, use the $timestamp and transform it back to the expected format return date('Y-m-d', $timestamp);

In order to do that you will need to change the willReturn to a callback, check how that is done here: https://stackoverflow.com/a/4711782

And as a final suggestion, before to upload a patch make sure that at least the tests that you are working on is still passing after your changes.

You can read here how to run the tests locally. and if you use PHPStorm it has a great integration

Remember, understanding the problem is half of the solution, always take your time and make sure to you understand what needs to be done.

Thank you for your work.

Please check this patch

This looks great!

Thank you.

+++ b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
@@ -150,13 +150,14 @@ public function testActiveTheme() {
+    $this->assertEquals($this->dateFormatter->format($timestamp,'html_date'), $result);

Let's not call the mock on the expected side of this assertion. Let's hard code the expectation and it to $this->assertEquals('1978-11-19', $result);. Which means we need to
change to $this->exactly(2) to $this->exactly(1) - which also points to this big the correct change as we're currently asserting that the test method is calling its own mock.

Re: #10
Hard coded the expectation & invoke method once.

Looks good.

Thanks!

Committed and pushed a0f43e0abd to 8.8.x and 8c4461de2e to 8.7.x. Thanks!

diff --git a/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
index 9b3e1a8f6a..6492b26b8b 100644
--- a/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
+++ b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
@@ -150,7 +150,9 @@ public function testActiveTheme() {
   public function testFormatDate() {
     $this->dateFormatter->expects($this->exactly(1))
       ->method('format')
-      ->will($this->returnCallback(function($timestamp) { return date('Y-m-d', $timestamp); }));
+      ->will($this->returnCallback(function ($timestamp) {
+        return date('Y-m-d', $timestamp);
+      }));
 
     $loader = new StringLoader();
     $twig = new \Twig_Environment($loader);

Fixed coding standards on commit.