Add unit tests for the Date service

Did you tried to use the static method using the trick described on http://sebastian-bergmann.de/archives/883-Stubbing-and-Mocking-Static-Me... ?

+++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
@@ -80,6 +80,15 @@ protected function prepareTimezone($timezone) {
@@ -146,10 +155,10 @@ public function format($format, $settings = array()) {

@@ -146,10 +155,10 @@ public function format($format, $settings = array()) {
+        static::formatDateCallback(NULL, $settings['langcode']);
...
-        $value = preg_replace_callback('/\xEF([AaeDlMTF]?)(.*?)\xFF/', '_format_date_callback', $format);
+        $value = preg_replace_callback('/\xEF([AaeDlMTF]?)(.*?)\xFF/', array(get_class($this), 'formatDateCallback'), $format);

We could make it accept a callable as argument.

I agree that this was a bit over the top.

sigh:( so much for getting rid of TestDate in the first place..
looks good to me and i ll rtbc once green

1. +++ b/core/lib/Drupal/Core/Datetime/Date.php
   @@ -185,4 +185,20 @@ protected function country() {
   +    return config_context_enter('Drupal\Core\Config\Context\LanguageConfigContext');
   

   Reading config_context_enter we should just inject the config.context.factory in here and use it

2. +++ b/core/lib/Drupal/Core/Datetime/Date.php
   @@ -185,4 +185,20 @@ protected function country() {
   +    config_context_leave();
   

   this is directly $this->configFactory->leaveContext()

Perfect, way better!

Does that mean that the unit test does not cover the entering into the config context?

There we go.

I am not sure a reroll is appropriate.

Lots has changed since November. This patch was committed. #2111349: Move format_plural to the string translation service and format_interval to the date service.
which notably introduced lots of tests. \Drupal\Tests\Core\Datetime\DateTest is already in core.

Although if people feel strongly the missing goals not covered by the existing tests could be augmented with test from this issue.

Otherwise should this issue be closed?

I think is still useful, as DateTest only tests formatInterval, and not format, which will be needed when format_date is removed. This should probably move the format_date tests over to DateTest.

Looks to me like the tests in core/modules/system/src/Tests/Common/FormatDateTest.php should be moved to core/tests/Drupal/Tests/Core/Datetime/DateTest.php.

In trying to clean this up I ran into a number of issues, and I think we may want to split up some of the behavior of DrupalDateTime. It's currently hard to test the DateFormatter (and format_date()) since it relies on DrupalDateTime::format(), which in turn relies on things like default/per user time zones and the string translation service. Seems like it would make more sense to break it up so that all the formatting work is done in the date formatting service, and DrupalDateTime just relies on that more testable service. Thoughts?

I think we can postpone this until we evaluate the state of things as part of #2543958: [META] DateTime Module Improvements.

Not sure what else is to be done here. The class this was opened for has been renamed to DateFormatter, which already has extensive unit test coverage in \Drupal\Tests\Core\Datetime\DateTest.

Closing this out.