Multisite unit tests and bp_blogs_record_existing_blogs()

[imath]

Each time this function is used in unit tests, for a weird reason users and blogs are not reset between each tests causing a large amount of tests to fail:

• because the domain of a blog created using $this->factory->blog->create() already exists
• because users created using $this->factory->user->create() are not reset.

I've noticed, they were already a hack into the BP_UnitTestCase->setUP method that was removing user 1. This was ok since each time bp_blogs_record_existing_blogs() was used only one user was created in the test. But as soon as you create more than one, you have the problem.

As i'm working on a new "split" of #6026 i need to test this function. I'm suggesting another work around as after 2 days on it, i still don't get what's happening.

To reproduce, simply test the bp_blogs_record_existing_blogs_unittest.patch using :
phpunit --group bp_blogs_record_existing_blogs -c tests/phpunit/multisite.xml
=> test is ok

Now run the complete suite :

phpunit -c tests/phpunit/multisite.xml
=>Dang! a large amount of errors and failures.

If you apply 6540.patch > no more errors or failures !

[boonebgorges]

Thanks for the digging, imath.

The root of the problem is this. Test fixtures, such as users, are created inside of a MySQL transaction. The WP test suite sets AUTOCOMMIT=0, which means that all writes are logged and then rolled back during tearDown() ('ROLLBACK;'). However, bp_blogs_record_existing_blogs() calls TRUNCATE, and TRUNCATE causes an implicit commit. As a result, any fixtures created before bp_blogs_record_existing_blogs() are committed to the database and not rolled back during tearDown().

There are a couple possible solutions.

1. Use DELETE instead of TRUNCATE in bp_blogs_record_existing_blogs(). This is, in fact, the way the function worked until [8585]. The commit message says that the purpose was to reset the auto-increment value. This reset does seem like a good thing, all things being equal, but it would be nice if johnjamesjacoby could chime in with more context.

2. Do a manual teardown of users, etc after every test. This is pretty much what we're already doing, and it's what imath's 6540.patch suggests.

3. Refactor existing tests that call bp_blogs_record_existing_blogs() so that they clean up after themselves.

Option 3 seems a lot more precise to me than 2. I'm going to work on a patch.

[boonebgorges]

6540.2.patch​ is an attempt at a patch that cleans up fixtures in a more targeted way, but with minimal requirements for the dev writing tests. Two parts:

• Add a hook at the end of bp_blogs_record_existing_blogs().
• In BP_UnitTestCase(), when 'bp_blogs_recorded_existing_blogs' fires, set an "autocommitted" flag. During tearDown(), if this flag has been set to true, manually delete user and blog data.

If need be, we could add additional content deletion to this block (like, say, groups). And set_autocommit_flag() could be used for other hooks, if we determined that other functions in BP were triggering autocommits.

[imath]

--no-prefix version of 2.patch

[imath]

Thanks a lot boonebgorges, i like it a lot! Just tested it :)

[boonebgorges]

In 9979:

Introduce bp_blogs_recorded_existing_blogs action.

See #6540.

[boonebgorges]

In 9980:

More targeted cleanup for unit tests that use bp_blogs_record_existing_blogs().

bp_blogs_record_existing_blogs() uses TRUNCATE, which closes the MySQL
transaction started by the WP automated test suite. As a result, fixtures
created before the TRUNCATE are not properly rolled back during tearDown().
Previously, the workaround for the problem had been heavy-handed: the wholesale
deletion of certain content during every test setup/teardown. The new technique
is to detect when a known autocommit has taken place - as it does in
bp_blogs_record_existing_blogs() - and to do a more targeted cleanup only in
these cases.

This allows us to dispense with various instances of blog fixture cleanup in
existing tests.

Fixes #6540.