Add unit tests for plugin.php

[sgrant]

I wrote some tests to expand the coverage for src/wp-admin/includes/plugin.php. In addition to the tests, there's a modification to a single return statement to send an empty array instead of null (it seemed strange to have a function returning two values--null or array()--when the list of plugins was unchanged). The only place that the modified function is used won't see any behaviour changes.

Please let me know if this looks reasonable, and thanks! (second ticket, still terrified) :)

[sgrant]

Unit tests and single change to return statement

[boonebgorges]

sgrant - Thanks so much for these tests.

It's not necessary to test _get_dropins() directly. For one thing, the values there are hardcoded. For another, the underscore prefix suggests that it's private, so that it'll get any necessary coverage from integration tests for functions that call _get_dropins(). I'll add a test that demonstrates this.

I'm also going to add expanded tests for get_mu_plugins().

These tests don't represent full coverage of the functions in question - for one thing, there should be more coverage of non-single-file plugins - but this is a great start.

[boonebgorges]

In 31002:

Improved tests for wp-admin/includes/plugin.php.

• Improved cleanup for existing tests, by ensuring that plugins are deactivated.
• New tests for get_plugin_files(), get_mu_plugins(), _sort_uname_callback(), get_dropins(), is_network_only_plugin(), activate_plugins(), validate_active_plugins(), and is_uninstallable_plugin().

Props sgrant.
See #30860.

[boonebgorges]

I've broken out the change to validate_active_plugins() and the corresponding test. The change looks fine to me, and I've searched through the repo to make sure there's no one doing anything that would be affected by the change in return value (there's not).

[boonebgorges]

In 31003:

When no plugins are active, return an empty array from validate_active_plugins().

This creates parity with the behavior of the function when plugins *are* active,
but none are invalid. It also makes it possible to write unit tests for the
function.

Props sgrant.
Fixes #30860.

[nacin]

The return value change here looks good to me. Sometimes we can't do this without breaking too much.

[jorbin]

We may still want to grep through the plugin and theme repos and make sure no one is using validate_active_plugins since this is a backwards incompatible change just to be on the safe side.

[boonebgorges]

Replying to jorbin:

We may still want to grep through the plugin and theme repos and make sure no one is using validate_active_plugins since this is a backwards incompatible change just to be on the safe side.

I did before committing. Only a small handful of plugins use the value, and all of them do a ! empty() check (mostly copypastaed from core), which is unaffected by the change. The only possible breakage would be someone doing a strict type check, like is_null(), but it's very difficult to imagine why this would ever be done in practice.

[valendesigns]

These tests fail if you have a db.php drop-in, like from Query Monitor for example. The issue is that deactivating Query Monitor does not remove the db.php drop-in, so I have to manually remove it and symlink the file back again to run the tests. I've written a patch that will account for the file, but I'm not sure what the protocol for this kind of thing is. Is anyone else in this boat with me?

[valendesigns]

Sorry, I should have clarified which tests are now failing.

1) Tests_Admin_includesPlugin::test_get_dropins_empty
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
+    'db.php' => Array (...)
 )

2) Tests_Admin_includesPlugin::test_get_dropins_not_empty
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => 'advanced-cache.php'
+    1 => 'db.php'
 )

[boonebgorges]

valendesigns - The unit tests are designed to run against a clean and unmodified version of WP. That said, we could adapt these tests to do a backup/restore technique for dropins, similar to the one that I wrote for mu-plugins. I'll leave the ticket open in case someone has a chance to get to this before I do.

[valendesigns]

@boonebgorges - Hopefully, my patch 30860.diff​ is what you had in mind. It solves the issue for me.

[valendesigns]

Hold up, one of the tests I wasn't running failed. Sorry I was only testing the plugin group. I'm hunting down the source now, then I'll add another patch.

[boonebgorges]

Yup, that's the idea, though you can probably simplify somewhat - since in the case of drop-ins we have a whitelist of file names, just do foreach ... if ( file_exists() ) rename ....

[valendesigns]

Replying to boonebgorges:

Yup, that's the idea, though you can probably simplify somewhat - since in the case of drop-ins we have a whitelist of file names, just do foreach ... if ( file_exists() ) rename ....

Good call, that really tidies things up.

So I've figured out the issue. If you have a symbolic link to db.php as one of your drop-in, Tests_DB:test_bail fails. However, everything works as expected and all tests pass when it's a real file.

[boonebgorges]

In 31009:

Back up and restore dropins during get_dropins() tests.

This change ensures that the get_dropins() tests don't detect any actual
dropins that you might be running on your develop.wordpress installation.

Props valendesigns.
Fixes #30860.