WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 13 months ago

#38381 closed defect (bug) (fixed)

Race conditions in Tests_Date_I18n

Reported by: pento Owned by: swissspidy
Milestone: 4.7 Priority: low
Severity: minor Version:
Component: Build/Test Tools Keywords: has-patch commit
Focuses: Cc:

Description

All of the tests that generate the expected and actual values from date() calls are subject to a race condition, where the time can tick to the next second between calls.

See this test job for an example of an inadvertent failure.

Attachments (2)

38381.diff (3.1 KB) - added by swissspidy 3 years ago.
38381.2.diff (4.2 KB) - added by swissspidy 3 years ago.

Download all attachments as: .zip

Change History (12)

#1 @pento
3 years ago

  • Summary changed from Race conditions in `Tests_Date_I18n` to Race conditions in Tests_Date_I18n

#2 @pento
3 years ago

  • Milestone changed from Future Release to 4.7
  • Owner set to swissspidy
  • Status changed from new to assigned

@swissspidy
3 years ago

#3 @swissspidy
3 years ago

  • Keywords has-patch added; needs-patch removed

@pento Thanks for opening the issue! I just saw it today and realized how bad the tests were. Mind a review?

#4 follow-up: @pento
3 years ago

To ensure consistent behaviour, the best option would be to get the output of time() at the start of each test, then pass it to the date() and date_i18n() calls, to ensure they're both generating from the same timestamp.

#5 in reply to: ↑ 4 @swissspidy
3 years ago

Replying to pento:

To ensure consistent behaviour, the best option would be to get the output of time() at the start of each test, then pass it to the date() and date_i18n() calls, to ensure they're both generating from the same timestamp.

But then I can't really test for the case where no timestamp is passed to the function. I mean, ideally we'd mock dates, but the delta comparison should work just as well.

#6 @pento
3 years ago

I think this'll be the first time we've ever used delta comparisons in our unit tests. :-)

Yah, let's go with it. Can you confirm that the $message parameter isn't required before you commit? The PHPUnit Docs imply that it is.

@swissspidy
3 years ago

#7 @swissspidy
3 years ago

  • Keywords commit added

Ah yeah you're right, the $message comes before the $delta. It's an empty string by default but I added a message in 38381.2.diff.

#8 @swissspidy
3 years ago

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

In 38871:

Date/Time: Fix unit tests after [38804].

The tests for date_i18n() need to use a delta for comparing timestamps.

Fixes #38381. See #37910.

#9 @netweb
3 years ago

Referencing here [39012] and [39013] follow up commits that were added to fix an issue with Daylight Savings Time ending in Europe, see https://wordpress.slack.com/archives/core/p1477789573011015

#10 @SergeyBiryukov
13 months ago

In 43562:

Tests: Avoid a race condition in test_date_i18n_handles_shorthand_formats() by using a delta for comparing timestamps.

See #20973, #38381.

Note: See TracTickets for help on using tickets.