Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 8 years ago

#23626 closed defect (bug) (fixed)

wp_convert_bytes_to_hr tests and NaN

Reported by: soulseekah's profile soulseekah Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 3.6 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch
Focuses: Cc:

Description

[1218/tests] were written for the deprecated wp_convert_bytes_to_hr, these tests are broken at least on 64-bit machines, at most - on all machines.

On 64-bit PHP (PHP_INT_SIZE == 8) floating point does not match expected output (gets 1022.9990234375MB). The last digits in floating point should not be depended upon due to these inaccuracies. Rounding in PHP < 5.3 is not an option either, since rounding mode was introduced later and different systems get different results (round up vs. round down by default). So perhaps looking at error and discrepancy would be a decent solution (maybe even less error 0.00001?).

Next [23440], in the function itself we get (int)double(NAN) result from feeding -1 and 0, which results in $power to be a huge negative number instead of 0. This results in both a Warning (undefined array index) being thrown, and the "B" missing in tests, i.e. expected "NANB" != "NAN" and expected "0B" != "0".

Related #19067

Attachments (6)

19067.function.patch (449 bytes) - added by soulseekah 12 years ago.
test_19067.patch (748 bytes) - added by soulseekah 12 years ago.
23626.patch (594 bytes) - added by SergeyBiryukov 12 years ago.
23626.test.patch (1.1 KB) - added by SergeyBiryukov 12 years ago.
23626.2.patch (781 bytes) - added by SergeyBiryukov 12 years ago.
23626.test.2.patch (1.2 KB) - added by SergeyBiryukov 12 years ago.

Download all attachments as: .zip

Change History (19)

#1 @kovshenin
12 years ago

  • Cc kovshenin added
  • Keywords has-patch added

#2 @SergeyBiryukov
12 years ago

  • Milestone changed from Awaiting Review to 3.6

#12915 suggested rounding the output to 2 decimal digits.

#22295 also mentioned the notice.

#3 @soulseekah
12 years ago

See: http://www.php.net/manual/en/function.round.php#98704 for PHP < 5.3 round direction is not predictable it seems.

#4 @SergeyBiryukov
12 years ago

In 1220/tests:

Fix wp_convert_bytes_to_hr() test for 64-bit systems. props soulseekah. see #23626.

#5 @SergeyBiryukov
12 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 23502:

Avoid an undefined offset notice in wp_convert_bytes_to_hr(). props soulseekah. fixes #23626.

#6 @SergeyBiryukov
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Still getting different results:

  • 32-bit, wp_convert_bytes_to_hr( -1 ): NANB
  • 64-bit, wp_convert_bytes_to_hr( -1 ): -1B

I didn't commit 19067.function.patch as is, because for 1 terabyte, the function would return "1B" (before [23502] it would be "1" and a notice).

Testing 23626.patch and 23626.test.patch now.

#7 @soulseekah
12 years ago

Perhaps it should return false for all sorts of weirdness? Or 0B.

Last edited 12 years ago by soulseekah (previous) (diff)

#8 @SergeyBiryukov
12 years ago

Replying to soulseekah:

Perhaps it should return false for all sorts of weirdness? Or 0B.

It's supposed to return a string. It also does not state that $bytes should be positive, so for "-1", I guess "-1B" is a more appropriate result than "0B".

We could also add "TB" to the array: 23626.2.patch, 23626.test.2.patch.

So far getting consistent results with the latest patches.

#9 @SergeyBiryukov
12 years ago

In 1226/tests:

Add more assertions for wp_convert_bytes_to_hr(). Correct expected result for a negative number. see #23626.

#10 @SergeyBiryukov
12 years ago

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

In 23551:

Make wp_convert_bytes_to_hr() return consistent results on 32-bit and 64-bit systems. fixes #23626.

#11 @SergeyBiryukov
11 years ago

In 1270/tests:

Fix wp_convert_bytes_to_hr() test for decimal comma locales. see #23626.

#12 @SergeyBiryukov
8 years ago

In 39265:

Tests: Use assertEquals()' native functionality for delta comparison in test_wp_convert_bytes_to_hr().

See #23626.

#13 @SergeyBiryukov
8 years ago

In 39267:

Tests: Add a missing $message argument for assertEquals() in [39265].

See #23626.

Note: See TracTickets for help on using tickets.