Make WordPress Core

Opened 8 years ago

Closed 2 years ago

Last modified 2 years ago

#37371 closed task (blessed) (fixed)

Reduce reliance on randomness in tests for 5.9

Reported by: johnbillion's profile johnbillion Owned by: johnbillion's profile johnbillion
Milestone: 5.9 Priority: lowest
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

39 of core's tests rely on rand_str() returning a different value each time it's called in order to pass. It's entirely possible that rand_str() could return an identical value twice, and therefore the test would randomly (ho ho) fail.

Let's remove all the unnecessary uses of rand_str(), where a fixed string would suffice. This will also shave off some milliseconds.

Attachments (2)

37371.patch (17.4 KB) - added by johnbillion 8 years ago.
37371.2.diff (71.5 KB) - added by johnillo 2 years ago.
Replace other rand_str() and rand() calls with static string to eliminate unnecessary randomness in tests. This is my first patch.

Download all attachments as: .zip

Change History (23)

@johnbillion
8 years ago

#1 @johnbillion
8 years ago

  • Keywords has-patch added; needs-patch removed

37371.patch replaces all instances of rand_str() with static strings where the test will fail if rand_str() were to return sequential identical strings.

#2 @johnbillion
8 years ago

  • Milestone changed from Future Release to 4.7

#3 @johnbillion
8 years ago

In 38382:

Build/Test Tools: Remove many unnecessary calls to rand_str() which can, in theory, fail at random. Static strings are much more appropriate.

See #37371

#4 @jorbin
7 years ago

@johnbillion Do you want to remove the rest of the calls to rand_strl? If so, good first bug?

#5 @johnbillion
7 years ago

  • Keywords needs-patch good-first-bug added; has-patch removed
  • Milestone changed from 4.7 to Future Release

#6 @johnbillion
7 years ago

  • Keywords good-first-bug removed
  • Owner set to johnbillion
  • Status changed from new to accepted
  • Summary changed from Reduce reliance on rand_str() in tests to Reduce reliance on randomness in tests

There's also a bunch of instances of rand() in tests which are just as unnecessary.

#7 @johnbillion
7 years ago

In 38762:

Build/Test Tools: Begin eliminating unnecessary randomness in tests.

Although unlikely, clashes in randomly generated strings could cause unexpected failures. In addition, most randomness is entirely unnecessary, is bad practice, and increases test time (however small it may be).

See #37371

#8 @johnbillion
7 years ago

In 38763:

Build/Test Tools: Continue eliminating randomness in tests.

See [38762]
See #37371

#9 @johnbillion
7 years ago

In 38938:

Build/Test Tools: Continue eliminating randomness in tests.

See #37371

#10 @johnbillion
7 years ago

In 39556:

Build/Test Tools: Remove some more randomness.

See #37371

#11 @desrosj
4 years ago

  • Keywords reporter-feedback added

@johnbillion is there any more work to be done here? Can we wrap this up during 5.4?

#12 @johnbillion
4 years ago

  • Keywords reporter-feedback removed
  • Milestone changed from Future Release to 5.4
  • Priority changed from normal to low

#13 @davidbaumwald
4 years ago

@johnbillion Is this still on your plate for 5.4?

#14 @johnbillion
4 years ago

  • Milestone changed from 5.4 to Future Release
  • Priority changed from low to lowest

#15 @johnbillion
3 years ago

  • Keywords good-first-bug added

@johnillo
2 years ago

Replace other rand_str() and rand() calls with static string to eliminate unnecessary randomness in tests. This is my first patch.

#16 @johnillo
2 years ago

  • Keywords has-patch added; needs-patch removed

#17 @johnbillion
2 years ago

  • Keywords good-first-bug removed
  • Milestone changed from Future Release to 5.9
  • Summary changed from Reduce reliance on randomness in tests to Reduce reliance on randomness in tests for 5.9
  • Type changed from enhancement to task (blessed)

I'm moving this into 5.9 to get the patch from @johnillo reviewed and committed before it goes too stale. This ticket can then be closed and any remaining unnecessary randomness can be addressed in follow-up tickets.

This ticket was mentioned in PR #2023 on WordPress/wordpress-develop by johnbillion.


2 years ago
#18

  • Keywords has-unit-tests added

Patch from johnillo. Opening a PR so we can verify that the tests pass.

Trac ticket: https://core.trac.wordpress.org/ticket/37371

#19 @johnbillion
2 years ago

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

In 52389:

Build/Test Tools: Reduce the use of unnecessary randomness in tests.

This increases the overall reliability of the tests.

Props johnillo

Fixes #37371

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


2 years ago

Note: See TracTickets for help on using tickets.