Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

#6540 closed defect (bug) (fixed)

Multisite unit tests and bp_blogs_record_existing_blogs()

Reported by: imath Owned by: boonebgorges
Milestone: 2.4 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch 2nd-opinion
Cc:

Description (last modified by 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 !

Attachments (4)

bp_blogs_record_existing_blogs_unittest.patch (1.8 KB) - added by imath 5 years ago.
6540.patch (1.5 KB) - added by imath 5 years ago.
6540.2.patch (7.2 KB) - added by boonebgorges 5 years ago.
6540.3.patch (7.2 KB) - added by imath 5 years ago.
--no-prefix version of 2.patch

Download all attachments as: .zip

Change History (10)

#1 @imath
5 years ago

  • Description modified (diff)

@imath
5 years ago

#2 @boonebgorges
5 years ago

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.
  1. 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.
  1. 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
5 years ago

#3 @boonebgorges
5 years ago

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
5 years ago

--no-prefix version of 2.patch

#4 @imath
5 years ago

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

#5 @boonebgorges
5 years ago

In 9979:

Introduce bp_blogs_recorded_existing_blogs action.

See #6540.

#6 @boonebgorges
5 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

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.

Note: See TracTickets for help on using tickets.