Remove failing automated tests from the suite

[boonebgorges]

Our previous convention was to write unit tests to demonstrate bugs, annotate those tests with a @ticket flag, and commit them to the repo. Then, when running the suite, WP_UnitTestCase::knownWPBug() would get a list of open tickets from Trac, and skip tests against open tickets, allowing the build to pass. This convention is not ideal for a couple reasons:

• It requires fetching a report from Trac on every run of the suite. This adds a couple seconds to each run of phpunit, which can be a real drag when using TDD techniques. (It also causes miscellaneous problems with transports and offline access - see ​https://buddypress.trac.wordpress.org/changeset/9053 for an illustration.)
• When you skip failed tests by default, you have to do things like phpunit --group 12345 to get them to run during development. This, in turn, can mask certain kinds of pollution between new and old tests, pollution that may not show up until after commit. See [30112] and [30148] for a couple of recent examples.
• In terms of developer psychology, committing a failing test is a way of washing one's hands of an issue: "look, I've documented the bug, and now I can move on".

The new convention is to commit tests only when fixing the bug. But this leaves dozens of failed tests in the repo. These tests should be pulled out, converted to patches, and uploaded to their relevant Trac ticket with a note indicating what's just happened. Once all the failed tests are out of the repo, we can remove the knownWPBug behavior from the test runner.

[jorbin]

I think for the failing tickets, we should reattach the tests to the existing ticket so that we don't lose them when/if they are fixed. One we have done that, we can remove the code that checks to see if the tickets are open. I say we start doing that and reference this ticket, along with the one that we are removing tests for in the all red commit.

I do think @ticket should be kept as a notation, but we also should generally be frugal with groups and make them as needed since they can help encourage test driven development.

[boonebgorges]

+1 to all of that. Thanks, jorbin.

[MikeHansenMe]

The zip I attached contains a bunch of patches removing tests with @ticket. I am going to go back through and add the tests back that have closed tickets and pass.

[boonebgorges]

Thanks, MikeHansenMe. This is a helpful start, but what would be *really* valuable is this: for each ticket that is currently being skipped, generate a patch that removes the test (that's what we'll commit) as well as another one that *adds* the tests (that's the one that will be attached to the open ticket for future reference). This is admittedly an easier workflow for a committer. But you could do it fairly easily with a git stash.

[MikeHansenMe]

#5305

[MikeHansenMe]

#6297

[MikeHansenMe]

#9930

[MikeHansenMe]

#10823

[MikeHansenMe]

#11946

[MikeHansenMe]

#14050

[MikeHansenMe]

#15448

[MikeHansenMe]

#16859

[MikeHansenMe]

#18897

[MikeHansenMe]

#19373

[MikeHansenMe]

#20043

[MikeHansenMe]

#21167

[MikeHansenMe]

#21169

[MikeHansenMe]

#21292

[MikeHansenMe]

#21319

[MikeHansenMe]

#22229

[MikeHansenMe]

#27326

[MikeHansenMe]

#28786

[MikeHansenMe]

#29557

[boonebgorges]

MikeHansenMe - Thanks for your continued work on this. I'm looking through these tests, and it looks like many of them are actually passing when forced to run. We don't need to remove passing tests from the suite - just those that fail because they demonstrate an unfixed bug. Sorry if that wasn't clear from the discussion above. (This, by the way, points to another problem with the "skipping" behavior: it causes tests to be skipped when they're good, passing tests, just because they're attached to a ticket that happens to be open for some reason.) So, a good technique for collecting these might be to set WP_FORCE_KNOWN_BUGS to true, and then start removing the tests that fail.

[boonebgorges]

In 30282:

Remove failing unit tests from 'canonical' group.

Each removed test has been turned into a patch and posted to the open ticket
that it belongs to.

See #30284.

[boonebgorges]

In 30283:

Improve Tests_Feed_RSS2::test_items().

• Better reference to post author, to avoid PHP notices.
• Code styling.
• More reliable checking of tags and categories.

Props kurtpayne.
Fixes ​#UT32. See #30284.

[boonebgorges]

In 30285:

Remove failing test related to post galleries.

It relied on the old _WPDataset technique, which is no longer used.

The test has been added to ​#UT30 as a patch, in case anyone wants to make
future use of it.

See #30284.

[jorbin]

In 30288:

Remove failing uploadfile test

The test has been added to #11946 and can be readded when that ticket is fixed.

Props MikeHansenMe for creating patch of current unit test

See #30284

[jorbin]

In 30289:

Remove failing uploadfile test in trunk

The test has been added to #11946 and can be readded when that ticket is fixed.

Props MikeHansenMe for creating patch of current unit test

See #30284

[jorbin]

In 30290:

Remove failing shortcode unit tests

Test added to #14050 in case we want to include it in a future fix.

see #30284

[jorbin]

In 30291:

Remove failing Unbalanced tags tests

Patch with tests added to #6297 for use in the future

Props MikeHansenMe for creating patch of current unit test

see #30284

[miqrogroove]

Do not delete test_unregistered_shortcodes(). If you have test feedback please put it on the appropriate ticket or message me directly on Slack.

[jorbin]

@microgroove - Keeping purposefully failing tests in the repo no longer makes sense. Back when we had two separate repositories and two separate tracs, it made sense to commit unit tests that we knew weren't going to pass. At that point, tests and our core code were essentially two separate projects. Now that we have combined them into one, it no longer makes sense. Our tests are as core to our project as any other piece of code we have.

That said, all the tests marked for 29557 are currently passing in trunk for me (even though the ticket is open for other reasons), so I have no intention of removing them. The work on this ticket is to make sure that we always run those tests even though the ticket is open (so we don't inadvertently break that code with other changes).

[miqrogroove]

The comments above said unit tests would be removed for all open tickets. Just wanted to clarify that.

[nacin]

In 30448:

Skip tests that were fixed in a later release. see #30284.

[nacin]

In 30451:

Skip tests that were fixed in a later release. see #30284.

[nacin]

In 30453:

Skip tests that were fixed in a later release. see #30284.

[nacin]

In 30454:

Skip tests that were fixed in a later release. see #30284.

[boonebgorges]

In 30513:

Remove failing tests related to WP_User::__unset().

These tests have been added as a patch to their original ticket, #20043.

Props MikeHansenMe.
See #30284.

[boonebgorges]

In 30514:

Remove failing tests related to square brackets being stripped in URL sanitizers.

These tests have been added as a patch to their original ticket, #16859.

Props MikeHansenMe.
See #30284.

[boonebgorges]

In 30515:

Improvements to encoded character formatting tests.

In sanitize_title_with_dashes() and sanitize_user() tests, we break large
test methods into smaller ones in order to isolate those that actually describe
the bug being reported in ticket #10823. The unit tests that are continuing to
fail have been attached as a patch to that ticket.

See #30284.

[boonebgorges]

In 30516:

Remove failing test for unimplemented wp_mail() enhancement.

The removed test has been added as a patch to the original ticket, #15448.

Props MikeHansenMe.
See #30284.

[boonebgorges]

In 30518:

Remove skipped tests for WP_Export_Query.

These tests have been added as a patch to the ticket where the new
functionality was originally proposed, #22435.

See #30284.

[boonebgorges]

In 30519:

Remove failing test in the hooks group.

The removed test has been added as a patch to #21169.

Props MikeHansenMe.
See #30284.

[boonebgorges]

In 30520:

Remove failing assertions from is_serialized() tests.

They have been moved to new test methods, which have been attached as a patch
to the ticket #9930.

See #30284.

[boonebgorges]

In 30521:

Remove failing is_textdomain_loaded() test.

The removed test has been added as a patch to the original ticket, #21319.

Props MikeHansenMe.
See #30284.

[boonebgorges]

In 30522:

Remove failing test related to wp_unique_post_slug().

The test has been added as a patch to #18962.

See #30284.

[boonebgorges]

In 30523:

Remove failing test related to wp_list_pages().

The test has been added as a patch to #27326.

Props MikeHansenMe.
See #30284.

[boonebgorges]

In 30524:

Remove failing test related to the 'offset' param of WP_Query.

This test has been added as a patch to #18897.

Props MikeHansenMe.
See #30284.

[boonebgorges]

In 30526:

Stop checking Trac to skip tests against open tickets.

In general, skipped tests should live only in patches, which are committed at
the same time that the corresponding bug is fixed. In cases where it's
necessary to skip a test, use markTestSkipped() to declare this fact
explicitly.

We continue to check Trac when using WP_UnitTestCase to run non-core tests.

See #30284.

[boonebgorges]

In 30527:

Ensure sanitize_user() expected test values are lowercase on multisite.

[30524] neglected to account for the fact that multisite forces user logins to
lowercase.

See #30284.

[boonebgorges]

In 30530:

Remove failing test_network_limit() XML-RPC test.

Test added as a patch on #21292.

Props MikeHansenMe.
See #30284.

[boonebgorges]

In 31253:

Remove skipped tests related to the proposed Export API.

These tests have been added as a patch to #22435.

See #30284.

[boonebgorges]

In 31254:

Remove WP_Post_IDs_Iterator tests.

The tests have been moved to a patch on the ticket where the class has been
proposed, #22435.

See #30284.

[boonebgorges]

In 31257:

Move GD-specific resize test to GD-specific resize test file.

This makes it so that we don't have to mark the test as skipped when running
through Imagick tests.

See #30284.

[boonebgorges]

All failing tests, and most skipped tests, have been removed from trunk. The only remaining skipped tests are those that depend on environment: tests that can only run with a certain PHP or Apache configuration, ones that can only run with/without Multisite, etc. This is a proper use of test skipping, so they stay.

There are a few places where Travis CI is skipping tests because the environment is not set up properly. This warrants a separate ticket. Marking this one resolved.

[johnbillion]

In 40449:

Build/Test Tools: Partial merge of [30283] into the 4.0 branch to avoid PHP notices that cause a test failure.

See #30284

[johnbillion]

In 40450:

Remove failing unit tests from 'canonical' group.

Each removed test has been turned into a patch and posted to the open ticket
that it belongs to.

See #30284.

Merges [30282] to the 4.0 branch.

See #40463

[johnbillion]

In 40452:

Remove failing test in the hooks group.

See #30284.

Merges [30519] to the 4.0 branch.

See #40463

[johnbillion]

In 40453:

Remove failing tests related to square brackets being stripped in URL sanitizers.

See #30284.

Merges [30514] to the 4.0 branch.

See #40463

[johnbillion]

In 40454:

Remove failing is_textdomain_loaded() test.

See #30284.

Merges [30521] to the 4.0 branch.

See #40463

[johnbillion]

In 40455:

Remove failing tests related to WP_User::__unset().

See #30284.

Merges [30513] to the 4.0 branch.

See #40463

[johnbillion]

In 41304:

Remove failing test related to post galleries in the 4.0 branch.

See #30284.

Merges [30285] to the 4.0 branch.