In core/modules/views/src/Tests/Handler/FieldDateTest.php there is a note with a @todo that says we need to add tests for the raw time span and time span. This issue is to track that it gets done.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task - adding a new test that was missing
Issue priority Adding new tests is just a Normal thing, not Major/Critical, who knows how long it's been missing.
Unfrozen changes Unfrozen because it only adds tests and documentation.
CommentFileSizeAuthor
#15 fieldDateTest-future_formatters-2488844-15.patch6.03 KBmarkie
#13 fieldDateTest-future_formatters-2488844-13.patch5.19 KBmarkie
#12 fieldDateTest-future_formatters-2488844-12.patch4.26 KBmarkie
#4 fieldDateTest-future_formatters-2488844-3.patch5.51 KBmarkie
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markie’s picture

markie CreditAttribution: markie at Mediacurrent commented

Just want to verify, do we want to write tests for all the default views date formatters? I have this as a list:

"fallback" => 'Fallback date format format: Fri, 05/15/2015 - 20:35',
"html_date" => 'HTML Date format: 2015-05-15',
"html_datetime" => 'HTML Datetime format: 2015-05-15T20:35:53+0000',
"html_month" => 'HTML Month format: 2015-05',
"html_time" => 'HTML Time format: 20:35:53',
"html_week" => 'HTML Week format: 2015-W20',
"html_year" => 'HTML Year format: 2015',
"html_yearless_date" => 'HTML Yearless date format: 05-15',
"long" => 'Default long date format: Friday, May 15, 2015 - 20:35', // <-- done
"medium" => 'Default medium date format: Fri, 05/15/2015 - 20:35', // <-- done
"short" => 'Default short date format: 05/15/2015 - 20:35', // <-- done (but fails)
"custom" => 'Custom', // <-- done
"raw time ago" => 'Time ago', // <-- done
"time ago" => 'Time ago (with "ago" appended)', //<-- done
"raw time hence" => 'Time hence', // <-- stubbed but fails if uncommented.
"time hence" => 'Time hence (with "hence" appended)', <-- stubbed but fails if uncommented.
"raw time span" => 'Time span (future dates have "-" prepended)',
"inverse time span" => 'Time span (past dates have "-" prepended)',
"time span" => 'Time span (with "ago/hence" appended)',
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

If all of those methods are lacking tests, then yes it would be good to add tests for all of them. Thanks!

markie’s picture

markie CreditAttribution: markie at Mediacurrent commented
Assigned: Unassigned » markie
markie’s picture

markie CreditAttribution: markie at Mediacurrent commented
Status: Active » Needs review
FileSize
fieldDateTest-future_formatters-2488844-3.patch5.51 KB

Here is my first pass at the "time hence" (future comparisons) tests. I had to add a new field to the view that has a future date and set it to 2050. I assume we'll be done by then.

Status: Needs review » Needs work

The last submitted patch, 4: fieldDateTest-future_formatters-2488844-3.patch, failed testing.

markie’s picture

markie CreditAttribution: markie at Mediacurrent commented
Status: Needs work » Active

So my fixed killed other tests. I forget if that's called progress or not.

markie’s picture

markie CreditAttribution: markie at Mediacurrent commented

So my fixed killed other tests. I forget if that's called progress or not.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

I think maybe some of those failures were random test bot glitches, so retesting.

jhodgdon’s picture

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

Oh, well maybe not, it looks like the array that you added to was being used in another test, so best to do make your own test data for this test. Shouldn't be too hard to update?

Status: Needs work » Needs review

jhodgdon queued 4: fieldDateTest-future_formatters-2488844-3.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 4: fieldDateTest-future_formatters-2488844-3.patch, failed testing.

markie’s picture

markie CreditAttribution: markie at Mediacurrent commented
Status: Needs work » Needs review
FileSize
fieldDateTest-future_formatters-2488844-12.patch4.26 KB

It makes more sense to do it all in one test instead of manipulating the main. That's the beauty of extending a class right?

markie’s picture

markie CreditAttribution: markie at Mediacurrent commented
FileSize
fieldDateTest-future_formatters-2488844-13.patch5.19 KB

Adding other time formats as listed in #1. Full array of tests should be complete now.

jhodgdon’s picture

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

Thanks! Looks good, and the test passes. The only problem I saw was one typo:

+++ b/core/modules/views/src/Tests/Handler/FieldDateTest.php
@@ -43,6 +71,13 @@ public function testFieldDate() {
+        'id' => 'destryoyed',

Typo: destryoyed => destroyed

Also, any method you add to a class should have a doc block, even in a test. I see that the others in that test class don't have doc blocks, but our Documentation Gate requires that any added code be added with documentation. So please add doc blocks to your added methods. If you want to go back and document the rest of the class, that would be a bonus; if not we can file a follow-up issue.

Doc block standards are at:
https://www.drupal.org/node/1354

markie’s picture

markie CreditAttribution: markie at Mediacurrent commented
Status: Needs work » Needs review
FileSize
fieldDateTest-future_formatters-2488844-15.patch6.03 KB

Updated docblocks and fixed typo.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Thanks!! Excellent! I could suggest a few minor wording changes in the docs, but since this is all tests and it's all clear, I think the docs are good enough -- and having doc blocks at all is a Big Plus.

Adding beta eval to issue summary.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Issue summary: View changes

Note: #2456521: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known patch will conflict with this, so we'll see which gets committed first (I'm marking this issue as related from there, since that is where this one came from).

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 9150446 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation.

  • alexpott committed 9150446 on 8.0.x 
    Issue #2488844 by markie, jhodgdon: Write tests for raw time span and...

Status: Fixed » Closed (fixed)

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