-
-
Notifications
You must be signed in to change notification settings - Fork 515
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: split test running for integrations (plugins) #2904
Conversation
@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
I didn't know we were returning Lines 196 to 199 in eecfb03
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? |
👍
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 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 Or just leave the check for empty results when getting all meta values and use // Empty result.
if (empty($object_meta)) {
$object_meta = empty($field_name) ? [] : $object_meta;
} |
Although this makes more sense to return null when no value was found, I think users might expect behavior parity with
This should be not be necessary if we remove the last Line 180 in f3401a5
I think we can safely remove the whole statement. |
@gchtr I updated the code. Let me know if this is ok for you, I'll update the tests accordingly. |
2f869cc
to
ceec9a2
Compare
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:
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.