Given the rather basic problems like #989894: Too many "Localize date formats" options and #989886: _system_date_format_types_build improper use of in_array which weren't really looked at until near the D7 release, it's clear that the date format system needs some more robust testing.

Hopefully this will also serve as a guide for properly creating date formats in a module, so further issues like #989366: format_date() doesn't use programmatically created format. can be more easily solved.

CommentFileSizeAuthor
#16 1013980-14-test-and-1015236-10.patch11.65 KBbfroehle
#14 1013980-14-tests-only.patch6.02 KBbfroehle
#14 1013980-14-tests-and-1015236-4.patch6.02 KBbfroehle
#5 1013980-5-Begin-work-on-new-date-time-unit-tests.patch7.52 KBbfroehle

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Date Format System Needs More Tests » Date types/formats need proper tests
Category: feature » task
Issue tags: +Needs tests
bfroehle’s picture

bfroehle commented
Assigned: bfroehle » Unassigned

Unit test would need coverage of:

Hooks:

hook_date_format_types
hook_date_format_types_alter
hook_date_formats
hook_date_formats_alter

Functions:

format_date
system_get_date_types
system_get_date_formats

Locate - related testing (which may already exist in locale.test ?).

system_date_format_locale
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Inventory of existing tests:

a) locale.test has LocaleDateFormatsFunctionalTest:
- Visits admin/config/regional/date-time/locale/[LANG_CODE]/edit and sets up date formats
- Creates a node
- Views the node in two languages, and verifies that the Submitted information changes format. This implicitly tests format_date() with 'medium' as the default type, and is a partial test of the locale language setup in locale_get_localized_date_format(). But it doesn't test any date types besides 'medium', and since all 3 built-in types are set to the same format, it doesn't really test the locale language setup very well (there could be clashes, for instance).

b) system.test has DateTimeFunctionalTest, with 3 test methods:
- testDateFormatConfiguration - visits admin/config/regional/date-time/formats and verifies you can do CRUD operations from the UI there. Doesn't verify that these formats can be used for anything.
- testDateTypeConfiguration - visits admin/config/regional/date-time and verifies you can do CRUD operations from the UI there. Doesn't test that the date types can be used for anything.
- testTimeZoneHandling - Uses variable_set to set the time zone for the site, and verifies that the displayed submitted date for a node changes accordingly. Doesn't test the UI.

I don't see any other date-related tests, off-hand... definitely agreed we could use more!

bfroehle’s picture

bfroehle commented

I've written basic unit tests for system_get_date_types and system_get_date_formats so far.

One issue sprung up: #1015236: system_get_date_formats() return and doc information is inconsistent.

I'll post code once it's cleaned up a bit.

bfroehle’s picture

bfroehle commented
Status: Active » Needs review
StatusFileSize
new 1013980-5-Begin-work-on-new-date-time-unit-tests.patch7.52 KB

As promised, unit tests for system_get_date_types and system_get_date_formats. The tests were written by reading the documentation and attempting to guess the output of each function. Any failures are due to incorrect documentation or an incorrect implementation of the function (or a mistake by me).

On my system, only failures so far relate to #1015236: system_get_date_formats() return and doc information is inconsistent.

Marking needs review so that the bot triggers, but status is probably best kept at active.

Status: Needs review » Needs work

The last submitted patch, 1013980-5-Begin-work-on-new-date-time-unit-tests.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

I'm not sure I agree with the approach here, which is exhaustive testing of exact matching of return values of a few functions.

a) It's exhausting as well as exhaustive to write this type of tests.
b) We're not doing this level of testing anywhere else in Drupal.
c) I am not sure of the value of having this type of test, as compared to the huge effort of writing it.
d) Putting the currently-chosen default formats in the test means that if this choice (which is somewhat subjective) changes in the future, the tests will also need to change.
e) If the rest of the Drupal API was tested in this way, our tests would take way too long to run.

So while I applaud your efforts in the abstract as being very comprehensive, maybe an approach more similar to the rest of Drupal testing would be more appropriate?

What I think would be more appropriate would be to stick to testing whether the APIs and UIs work:
- if you use hook_date_formats to define a date format, does that date format show up as a choice on the formats admin page?
- if you use hook_date_format_types to define a date type, does that type show up as a choice on the types admin page?
- if you assign a format to a type from the UI, and assign some kind of a date in Drupal to display with that format, does it work?

bfroehle’s picture

bfroehle commented

jhodgdon: Agreed, this was intended to just be the very beginnings of a test suite.

Marking needs review so that the bot triggers, but status is probably best kept at active.

bfroehle’s picture

bfroehle commented
Status: Needs work » Active
bfroehle’s picture

bfroehle commented

Another date time issue that might be worth testing: #1012620: Unique key on date_formats.(format|type) is problematic for case insensitive collations

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

I'm just suggesting that the proposed tests are not the right tests, so that before you go write some more along the same lines, maybe your energy could be channeled in a better direction. And I'm not even sure we want to add those particular tests to the official Drupal test suite...

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Hm. In fact, we're doing such true unit tests elsewhere; see CRUD functionality tests in filter.test.

However, we shouldn't test embedded code. As always, there should be a hidden date_test.module, which registers a new dummy date type and date format, and the unit test should verify that the proper information is recorded, stored in the DB, locked types/formats cannot be updated, custom types/formats can be created and updated, and lastly, that the date format can be actively used, aso.

bfroehle’s picture

bfroehle commented

Another bug found from this: #1030788: hook_date_formats_alter() API sample code broken. More testing code soon.

bfroehle’s picture

bfroehle commented
Status: Active » Needs review
StatusFileSize
new 1013980-14-tests-and-1015236-4.patch6.02 KB
new 1013980-14-tests-only.patch6.02 KB

Implemented a custom test_date module which defines two date types/formats -- very short and extra large. The extra large type gets altered using the hooks. Comments appreciated.

Since the testing code is broken until #1015236-4: system_get_date_formats() return and doc information is inconsistent lands, I've included that patch in with one of the attachments here.

Status: Needs review » Needs work

The last submitted patch, 1013980-14-tests-and-1015236-4.patch, failed testing.

bfroehle’s picture

bfroehle commented
Status: Needs work » Needs review
StatusFileSize
new 1013980-14-test-and-1015236-10.patch11.65 KB

I suppose when you forget to actually include the referenced patches, things are destined for failure. Trying again.. test from #14 with patch from #1015236-10: system_get_date_formats() return and doc information is inconsistent.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Hm. So which lines of the patch actually fix the bug? There's a lot in that patch, and I'm confused about which code changes are actually necessary and which are cosmetic.

bfroehle’s picture

bfroehle commented

The attachment in #16 is equal to 1013980-14-tests-only.patch (from #14 in this thread) PLUS 1015236-10.patch (from #1015236-10: system_get_date_formats() return and doc information is inconsistent).

The first part is the testing routine, the second is the 'bug fix'.

On HEAD, the test routine fails because system_get_date_formats() does not return (exactly) the fields specified in the comments for those functions (as we've discussed in #1015236: system_get_date_formats() return and doc information is inconsistent). That could be easily changed if the 'module' field was instead to be omitted from system_get_date_formats()... by, for example, removing the 'module' => 'date_test' lines from the test routine.

Lastly, these tests are not as thorough as what sun suggests in #12 --- instead they merely do something similar to the first patch I included in #5 but with a "date_test" module and date formats defined there instead of the system provided date formats.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

I understand what the patch is composed of, but the "bug fix" portion has a bunch of stuff in it (such as doc fixes), and I'm sure not all of it is necessary to make the tests pass?

And am I understanding that the test is just failing because you have said in the test that the function should return 'module'? If so, let's just take that out of the test and go back to the doc fix on the other issue, since as we've discussed on that other issue, 'module' is not being used anywhere.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

-1 for removing 'module'. Perhaps not used in core, but it's a very useful information for contrib/custom modules.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

We are not removing it -- it was never added in the first place. The question is whether to remove it from the *docs* because the docs say it is there although it isn't.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.