Make WordPress Core

Opened 7 years ago

Last modified 5 years ago

#42904 new enhancement

Speed up unit tests by disabling password hashing

Reported by: frank-klein's profile Frank Klein Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch needs-refresh
Focuses: Cc:


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 7 years ago.
42904-2.patch (4.1 KB) - added by Frank Klein 7 years ago.

Download all attachments as: .zip

Change History (13)

@Frank Klein
7 years ago

#1 @Frank Klein
7 years ago

  • Keywords has-patch added

#2 @johnbillion
7 years 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.

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
7 years 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.

@Frank Klein
7 years ago

#4 @Frank Klein
7 years 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.

#5 @jorbin
6 years ago

  • Milestone changed from 5.0 to 5.1

#6 @pento
6 years ago

  • Keywords needs-refresh added; 2nd-opinion removed

42904-2.patch looks good. It needs an @since bump, but is otherwise fine to commit.

#7 @pento
6 years ago

  • Milestone changed from 5.1 to 5.2

Actually, I just got back to this ticket. It seems that WP_UnitTest_Factory_For_User::create_object() is only allowing it for tests that pass skip_password_hash, but currently no tests are doing that. Disabling that check gives the speed boost, but causes a bunch of errors.

So, I like the direction of 42904-2.patch, but it needs to be updated to turn off password hashing on the tests that don't need it.

#8 @desrosj
5 years ago

@frank-klein are you able to refresh this based on the feedback above? 5.2 beta 1 is in less than 2 days, but this is a test related change, so it could land during the beta. Leaving it in the milestone for now.

#9 @desrosj
5 years ago

  • Milestone changed from 5.2 to 5.3

Punting this, but it can be moved back if ready for 5.2.

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

5 years ago

#11 @desrosj
5 years ago

  • Milestone changed from 5.3 to Future Release

This still needs a refresh and more work. With 5.3 beta 1 in 3 days, I'm going to punt this.

Note: See TracTickets for help on using tickets.