Improved coverage for WP_Tax_Query unit tests

[boonebgorges]

See #29560 for a related ticket.

The quality and coverage of WP_Tax_Query and WP_Query::tax_query tests is pretty lackluster. This is problematic because it hinders future refactoring, and in particular it hamstrings the taxonomy roadmap.

The attached patch is an initial attempt to introduce some sanity to these tests. I've done the following:

• Written systematic tests for all public methods of WP_Tax_Query. Coverage here is pretty much complete.
• Moved the tests in tests/phpunit/term/query.php that have to do with WP_Query into tests/phpunit/post/query.php. The latter file is kind of a junk drawer of tests, and should probably be broken up, but at least my changes here make it consistent.
• Add some more WP_Query::tax_query tests, to cover a number of possible permutations: single vs multiple queries using AND and OR as the relation; various values for 'field'; various values for 'operator'
• Clean up existing tests for speed, relevance, better method names.

As in the case of WP_Meta_Query, the infinite permutations of query args make it impossible to write 100% test coverage, but this is a reasonable start.

[DrewAPicture]

Well, I'm impressed. Nice work.

One thing I wonder about is whether we should embrace the idea of testing the "any" taxonomy instead of pigeon-holing our tests to specifically categories or tags.

As usage of custom post types/taxonomies continues to grow, it might be worth taking a more dynamic tack to testing taxonomy APIs using random, factory-generated taxonomies (hierarchical or non-hierarchical as needed) whenever possible.

I think my hesitation to using built-in taxonomies stems from the knowledge that there's a great deal of institutional and legacy support for those taxonomies in core. Sometimes we need to specifically test categories and tags, and that's a given. I just don't think that means we always should.

[boonebgorges]

it might be worth taking a more dynamic tack to testing taxonomy APIs using random, factory-generated taxonomies (hierarchical or non-hierarchical as needed) whenever possible.

Thanks for this idea. I had the same basic thought when looking at these tests, but decided to go the conservative route for the time being. That being said, the general principle here ought to be that you should be testing as few things as possible per test. In the case of taxonomies, you correctly note that built-in taxonomies are "special" in terms of their core treatment. This suggests that the baseline is a generated (non-built-in) taxonomy, with 'post_tag' and 'category' being tested only when there is a reason to believe that they differ from the baseline.

I'm not sure whether it makes technical sense to have a factory for taxonomies, given that (a) they're not database objects, and (b) they pollute the global and so need a different cleanup method than a MySQL transaction rollback. So I'd suggest that, for now, we can do the following in any relevant test (or even setUp()/tearDown() class methods):

register_taxonomy( 'foo', 'post' );

// test

_unregister_taxonomy( 'foo' );

If others prefer this, I can rewrite the patch to use this technique instead of 'post_tag' and 'category'.

[boonebgorges]

29718.02.patch is a refresh that adds tests for this lovely bit of backward-compatibility: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/query.php?annotate=blame#L2701 (as introduced to address #12659). Note that a couple of these tests are marked @ticket 29718 because they're currently failing. I believe that these failures are due to bugs in the existing code (not checking an array index before using it, and not checking all meta_query clauses before setting the category- and tag-related vars). I'll write up a separate ticket soon for these bugs (and in fact they'll be fixed as part of a larger rewrite I'm working on), but for now I wanted to ensure that all the relevant tests were included in a single patch so that they'd apply cleanly.

[boonebgorges]

In 29805:

Improve unit tests for WP_Tax_Query.

• Exhaustive tests for publicly available functionality of WP_Tax_Query.
• For tests that are related to the tax_query argument as used in WP_Query, move to tests/post/query.php.
• Add some tax_query tests to cover single vs multiple queries using AND and OR; various values for 'field'; various values for 'operator'.
• Improve test names.
• Correct @group annotations.
• Improve performance of some WP_Query-related tests by declaring 'update_post_meta/term_cache' false.

Fixes #29718

[boonebgorges]

Going to reopen this ticket because a couple of the tests are failing, and having the ticket open will make sure they're skipped on a default run of the tests. The failing tests are the ones I mention here https://core.trac.wordpress.org/ticket/29718#comment:3 but I forgot to remove them before committing. I'll open up a new ticket and move them over.

[jorbin]

Tests_Post_Query::test_tax_query_single_query_multiple_terms_operator_in is inconsistantly failing due to the ordering of the results. As this is not consistant, i tend to think it is a timing issue on travis-ci and not an actual bug.

The above patch uses assertEqualSets since what matters isn't the order as much as the results.

[boonebgorges]

Change looks good to me.

[jorbin]

In 29857:

Change assertEqual to assertEqualSets since the order that travis-ci creates posts isn't consistent

see #29718

[boonebgorges]

In 29891:

Introduce support for nested queries in WP_Tax_Query.

Previously, tax query arguments could be joined by a single AND or OR relation.
Now, these queries can be arbitrarily nested, allowing clauses to be linked
together with multiple relations.

In a few places, WP_Query runs through a list of clauses in a tax_query in order
to set certain query vars for backward compatibility. The necessary changes have
been made to WP_Query to support this feature with the new complex structure of
tax_query. Unit tests are included for these backward compatibility fixes.

Unit tests for the new nesting syntax are included.

Props boonebgorges.
Fixes #29718. See #29738.

[nofearinc]

While testing the latest trunk I and @mordauk got a Fatal Error on a theme running on my blog. Xdebug stack trace here said the following:

( ! ) Fatal error: Cannot break/continue 1 level in /opt/lampp/htdocs/wpbleeding/wp-includes/taxonomy.php on line 1044
Call Stack
#	Time	Memory	Function	Location
1	0.0001	329472	{main}( )	../index.php:0
2	0.0001	332584	require( '/opt/lampp/htdocs/wpbleeding/wp-blog-header.php' )	../index.php:17
3	0.3819	24099580	wp( )	../wp-blog-header.php:14
4	0.3819	24099652	WP->main( )	../functions.php:873
5	0.3848	24156160	WP->query_posts( )	../class-wp.php:612
6	0.3848	24156232	WP_Query->query( )	../class-wp.php:541
7	0.3849	24156832	WP_Query->get_posts( )	../query.php:3843
8	0.3885	24167100	WP_Tax_Query->get_sql( )	../query.php:2685
9	0.3885	24167224	WP_Tax_Query->get_sql_clauses( )	../taxonomy.php:857
10	0.3885	24167384	WP_Tax_Query->get_sql_for_query( )	../taxonomy.php:882
11	0.3886	24168808	WP_Tax_Query->get_sql_for_clause( )	../taxonomy.php:933

The changeset here calls continue; in the new get_sql_for_clause() function, but no loop has been defined within the function. This seems to cause the fatal error on PHP 5.3.x (two different versions).

That might as well work in 5.4 and above, but seems to be a blocker in earlier versions. We could probably avoid continue; by reversing the conditional as in the patch attachment.

[nofearinc]

reverse conditionals

[boonebgorges]

nofearinc - Thanks for the report. This appears to be leftover logic from the old loop format of get_sql() - totally my fault. I think an easier fix than reversing the logic of each clause is just to return the empty $sql array in these cases. I'll run some tests.

[nofearinc]

where clause could be empty

[nofearinc]

@boonebgorges sure thing. In case the patch update is fine, added an empty $where at the beginning since it may not have been initialized if the terms array is empty.

[boonebgorges]

In 29931:

Remove invalid continue calls from WP_Tax_Query::get_sql_for_clause().

This was leftover code from the previous implementation, which used a foreach()
loop. See [29901].

Props nofearinc.
See #29738, #29718.

[boonebgorges]

Fixed in [29931] (and verified by unit tests :) ). Sorry for the issue - if there are more problems with this refactor, let's put them over on #29738. Thanks!