test: split test running for integrations (plugins)

[nlemoine]

Issue

This is something I want to tackle since quite some time.

I have an integration locally ready for Authorship. Since this a replacement for CoAuthors, running tests failed because you can't run them with both plugins enabled.

This made me notice we actually run ALL tests with all plugins enabled (currently ACF & CoAuthors).

This could lead to errors (it does but I didn't figure why for all of them), among those:

• usage of ACF functions (get/udpate_field) instead of get/update_{$type}_meta (they do not behave the same way)
• duplicated tests

Solution

Split test runs and only test the plugin group seprated from the rest of tests.

Note that this a work in progress, I still wanted to open the PR so you could have a look too.

Considerations

Unfortunately, I couldn't find a way to isolate plugins tests in a single run. I don't think that's even possible considering the WordPress test lib design.

[nlemoine]

@gchtr As discussed today, I removed the tests related to revisions.

The last test that does not pass is interesting and points to something I wasn't aware of 😅

timber/tests/test-timber-term.php

Lines 581 to 590 in eecfb03

	public function testTermWithNativeMetaFalse()
	{
	$tid = $this->factory->term->create([
	'name' => 'News',
	'taxonomy' => 'category',
	]);
	add_term_meta($tid, 'foo', false);
	$term = Timber::get_term($tid);
	$this->assertSame('', $term->meta('foo'));
	}

I didn't know we were returning null if the meta value was evaluated to false by empty (a practice that some people advise against).

timber/src/CoreEntity.php

Lines 196 to 199 in eecfb03

	// Empty result.
	if (empty($object_meta)) {
	$object_meta = empty($field_name) ? [] : null;
	}

I don't where we should stand. I'm quite fine with this but I'm just wondering if it could have side effects.

Thoughts?

[gchtr]

@gchtr As discussed today, I removed the tests related to revisions.

👍

The last test that does not pass is interesting and points to something I wasn't aware of 😅

So what would be the ideal case here? Should we return whatever the term value is going to be, right?

I introduced this in 2.0 (https://github.com/timber/timber/pull/1904/files#diff-4c2254ab4c36798ab8f6cf02ca8941343e9f17b57fe7816ecc1d1744a1506487R257-R260), so side effects are that certain checks are breaking when people upgrade 2.0.

I can’t remember why I did it this way. Maybe I thought I was clever. Maybe I wanted to return an empty array if no meta is set on a post and then didn’t consider the fallback return value enough. But in this case, falsey values are what we might expect. If you use a meta field for a boolean option, you’d expect true and false to be returned, not true and null.

If this error surfaces now that we run tests without integration, it’s a good hint that we could consider this as a bug. Intuitively, I’d say we remove the whole check and change the return type to mixed.

Or just leave the check for empty results when getting all meta values and use $object_meta as a fallback value.

// Empty result. 
if (empty($object_meta)) { 
     $object_meta = empty($field_name) ? [] : $object_meta; 
} 

[nlemoine]

So what would be the ideal case here? Should we return whatever the term value is going to be, right?

Although this makes more sense to return null when no value was found, I think users might expect behavior parity with get_{type}_meta. If one would like another behavior (like returning null for empty values), then filters can be used for this purpose.

Or just leave the check for empty results when getting all meta values and use $object_meta as a fallback value.

This should be not be necessary if we remove the last !empty condition here:

timber/src/CoreEntity.php

Line 180 in f3401a5

if (empty($field_name) && \is_array($object_meta) && !empty($object_meta)) {

I think we can safely remove the whole statement.

[nlemoine]

@gchtr I updated the code. Let me know if this is ok for you, I'll update the tests accordingly.