WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 10 months ago

#34378 closed enhancement (fixed)

unit tests for the current_time() in functions.php

Reported by: pbearne Owned by: SergeyBiryukov
Milestone: 5.4 Priority: normal
Severity: normal Version: 4.4
Component: Date/Time Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

just some unit tests

Attachments (3)

current-time.php.patch (4.1 KB) - added by pbearne 5 years ago.
Unit tests
currentTime.php.patch (3.3 KB) - added by pbearne 10 months ago.
updated file
34378.diff (1.8 KB) - added by SergeyBiryukov 10 months ago.

Download all attachments as: .zip

Change History (12)

@pbearne
5 years ago

Unit tests

#1 @pbearne
5 years ago

  • Keywords needs-patch has-unit-tests added
  • Summary changed from unit tests for current_time() to unit tests for the current_time() in functions.php

#2 @pbearne
5 years ago

  • Keywords has-patch added; needs-patch removed

This ticket was mentioned in Slack in #core by noisysocks. View the logs.


10 months ago

#4 @talldanwp
10 months ago

  • Component changed from General to Date/Time
  • Keywords needs-patch added; has-unit-tests has-patch removed
  • Milestone set to 5.4

Hi @pbearne, thanks so much for working on this! Tests are always welcome.

I applied the patch and ran the tests, and it looks like there's one failing test:

1) Tests_Functions_current_time::test_current_time_pass_random_String
Random_String passed
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Ram116202001_th31Thu, 16 Jan 2020 05:53:23 +00005315_pam2323UTC16'
+'Ram116202001_th31Thu, 16 Jan 2020 05:53:23 +00005315_pam2323+00:0016'

This would need to be addressed. A couple of other points looking at code consistency:

  • Most other test filenames are camelCased instead of kebab-cased.
  • The documentation at the top of the file seems to be the docs for the function. I don't think this is needed.

#5 @pbearne
10 months ago

Its been a while since I wrote this and TIME/DATE got fixed so I will revisit ASAP and get a new patch with the cleaned code

@pbearne
10 months ago

updated file

#6 @pbearne
10 months ago

  • Keywords has-patch has-unit-tests added; needs-patch removed
  • Version changed from 4.4 to trunk

Patch cleaned up
I removed the broken test as it is not needed

#7 @SergeyBiryukov
10 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing
  • Version changed from trunk to 4.4

#8 @SergeyBiryukov
10 months ago

Thanks for the patch!

Just noting that some tests for current_time() were previously added in [43594].

34378.diff cleans up some duplicate tests and brings the rest in line with the existing ones, including using delta comparison to avoid race conditions like #38381, #38815, #45821, #48145.

These still seem a bit redundant, as some of these cases are covered by the existing tests and there should be no functional difference between gmt_offset and timezone_string modes, per comment:9:ticket:48384.

That said, I guess it's a good idea to have some tests for gmt_offset too. Thanks again!

#9 @SergeyBiryukov
10 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 47081:

Date/Time: Add some basic tests for current_time().

Props pbearne, talldanwp, SergeyBiryukov.
Fixes #34378.

Note: See TracTickets for help on using tickets.