WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 6 months ago

#42904 new enhancement

Speed up unit tests by disabling password hashing

Reported by: Frank Klein Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description

Whenever the factory creates a new user during a test, it calls wp_insert_user(). This function calls wp_hash_password(), which internally uses PasswordHash::HashPassword() to create a hash of the default password.

In the context of most unit tests, users do not need hashed passwords. Hashing has a performance impact, so the tests should run a bit faster if we avoid hashing if we don't need to.

The attached patch introduces a mock password hasher, that is used by the entirety of the tests The exception are a few tests that rely on authentication to work properly.

Attachments (2)

42904.patch (4.4 KB) - added by Frank Klein 6 months ago.
42904-2.patch (4.1 KB) - added by Frank Klein 6 months ago.

Download all attachments as: .zip

Change History (6)

@Frank Klein
6 months ago

#1 @Frank Klein
6 months ago

  • Keywords has-patch added

#2 @johnbillion
6 months ago

  • Keywords 2nd-opinion added
  • Milestone changed from Awaiting Review to 5.0

Some background: The PasswordHash class used for password hashing calls md5() repeatedly to generate a portable hash. This accounts for millions of calls to md5() and by far the greatest number of calls to a single function when running the full test suite.

https://pbs.twimg.com/media/CdtV1nCXIAAqMgC.jpg

That said, the combined wall time of md5() is not that high, taking up around 3% of wall time from my testing. 42904.patch works and the tests pass, but the time saving unfortunately isn't as great as I hoped it would be. The time saving on my local machine is around 15 seconds reduction on a test suite run that takes 1m 40s.

#3 @boonebgorges
6 months ago

15 seconds reduction off of 100s total is a pretty big win, IMO. I don't see the downside.

One thing I'd suggest is that the $GLOBALS juggling could happen inside of the User factory class. Perhaps the create() method could accept a skip_password_hash param that would default to false. This would make it more transparent how to override when a test case needs a hashed pass.

#4 @Frank Klein
6 months ago

Thanks for the feedback @boonebgorges, I adapted the patch as proposed.

I also added a unit test for this functionality of the user fixtures factory. It is excluded by default, as I don't see the need to test the factory every time the suite runs.

I did not add the new argument to the inline documentation of the user fixtures factory, as it is not a straightforward change to make. Something for another ticket.

Note: See TracTickets for help on using tickets.