WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#30860 closed enhancement (fixed)

Add unit tests for plugin.php

Reported by: sgrant Owned by: boonebgorges
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.2
Component: Build/Test Tools Keywords: needs-patch
Focuses: Cc:

Description

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) :)

Attachments (3)

patch.diff (4.3 KB) - added by sgrant 5 years ago.
Unit tests and single change to return statement
30860.diff (2.5 KB) - added by valendesigns 5 years ago.
30860.1.diff (2.1 KB) - added by valendesigns 5 years ago.

Download all attachments as: .zip

Change History (18)

@sgrant
5 years ago

Unit tests and single change to return statement

#1 @boonebgorges
5 years ago

  • Milestone changed from Awaiting Review to 4.2

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.

#2 @boonebgorges
5 years ago

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.

#3 @boonebgorges
5 years ago

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).

#4 @boonebgorges
5 years ago

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

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.

#5 @nacin
5 years ago

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

#6 follow-up: @jorbin
5 years ago

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.

#7 in reply to: ↑ 6 @boonebgorges
5 years ago

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 function, 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.

Last edited 5 years ago by boonebgorges (previous) (diff)

#8 @valendesigns
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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?

#9 @valendesigns
5 years ago

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'
 )

#10 follow-up: @boonebgorges
5 years ago

  • Keywords needs-patch added

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

#11 in reply to: ↑ 10 @valendesigns
5 years ago

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

#12 @valendesigns
5 years ago

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.

#13 @boonebgorges
5 years ago

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

#14 @valendesigns
5 years ago

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.

#15 @boonebgorges
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.