Unit tests: super admin privledges leak to other tests

[jdgrimes]

If you create a user during a test and grant them super admin privileges, users created in the following tests will also be super admins.

Example:

class Example_Super_Admin_Test extends WP_UnitTestCase {

	/**
	 * This will work as expected.
	 */
	public function test_one() {
		$user_id = $this->factory->user->create();
		grant_super_admin( $user_id );
		$this->assertTrue( is_super_admin( $user_id ) );
	}
	
	/**
	 * This will fail.
	 */
	public function test_two() {
		$user_id = $this->factory->user->create();
		$this->assertFalse( is_super_admin( $user_id ) );
	}
}

The reason is because the factory assigns these two users the same user_login, 'user 1'. But the $super_admins global is not cleared at the end of each test, and so once 'user 1' is added as a super admin in the first test, all users created with that login in the following tests will also have super admin privileges.

The solution is to reset the $super_admins global in our tearDown(), or change the behavior of get_super_admins() to call the database/cache each time instead of relying on the global.

[jdgrimes]

Query the db/cache each time in get_super_admins()

[jdgrimes]

Do you think 27205.diff​ is the way to go?

[SergeyBiryukov]

Replying to jdgrimes:

Do you think 27205.diff​ is the way to go?

Per [14206] and #12815, $super_admins is meant to be set as an override in wp-config.php.

Looks like 27205.diff​ would break grant_super_admin() and revoke_super_admin().

[jdgrimes]

Replying to SergeyBiryukov:

Replying to jdgrimes:

Do you think 27205.diff​ is the way to go?

Per [14206] and #12815, $super_admins is meant to be set as an override in wp-config.php.

Looks like 27205.diff​ would break grant_super_admin() and revoke_super_admin().

Ah, OK. I'll see if I can work up a patch for just the test framework then.

[jdgrimes]

Unset $GLOBALS['super_admins'] on setUp()

[jdgrimes]

New patch approaches this from the other angle.

[nacin]

Well this is embarrassing. grant_super_admin() only works once per pageload. It checks the $super_admins global and bails if it is set, then proceeds to set it. 27205.3.diff​ is a unit test that fails.

get_super_admins() consults $super_admins directly, but as this function is called fairly often, I wonder if this was accidentally done for speed purposes.

[jdgrimes]

Don't overwrite the $super_admins global in *_super_admin()

[jdgrimes]

Use $GLOBALS['super_admins'] insteaad of bringing it into the current scope

[jdgrimes]

Replying to nacin:

Well this is embarrassing. grant_super_admin() only works once per pageload. It checks the $super_admins global and bails if it is set, then proceeds to set it. 27205.3.diff​ is a unit test that fails.

get_super_admins() consults $super_admins directly, but as this function is called fairly often, I wonder if this was accidentally done for speed purposes.

revoke_super_admin() is also contributing to this problem. 27205.5.diff​ addresses this by checking $GLOBALS['super_admins'] istead of bringing the $super_admins into the functions' scopes. (Ignore 27205.4.diff​)

[nacin]

In 27706:

Multisite: Don't set the $super_admins global in grant_super_admin(), revoke_super_admin().

Adds tests and docs.

props jdgrimes.
see #27205.

[nacin]

[27706] solves the leakage issue as well.