WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 2 years ago

#30284 closed task (blessed) (fixed)

Remove failing automated tests from the suite

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

Description

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.

Attachments (20)

30284.zip (88.5 KB) - added by MikeHansenMe 5 years ago.
5305-remove.diff (1.1 KB) - added by MikeHansenMe 5 years ago.
#5305
6297-remove.diff (2.8 KB) - added by MikeHansenMe 5 years ago.
#6297
9930-remove.diff (1.1 KB) - added by MikeHansenMe 5 years ago.
#9930
10823-remove.diff (2.3 KB) - added by MikeHansenMe 5 years ago.
#10823
11946-remove.diff (1002 bytes) - added by MikeHansenMe 5 years ago.
#11946
14050-remove.diff (860 bytes) - added by MikeHansenMe 5 years ago.
#14050
15448-remove.diff (1.3 KB) - added by MikeHansenMe 5 years ago.
#15448
16859-remove.diff (2.0 KB) - added by MikeHansenMe 5 years ago.
#16859
18897-remove.diff (913 bytes) - added by MikeHansenMe 5 years ago.
#18897
19373-remove.diff (2.6 KB) - added by MikeHansenMe 5 years ago.
#19373
20043-remove.diff (1.3 KB) - added by MikeHansenMe 5 years ago.
#20043
21167-remove.diff (3.1 KB) - added by MikeHansenMe 5 years ago.
#21167
21169-remove.diff (1.1 KB) - added by MikeHansenMe 5 years ago.
#21169
21292-remove.diff (1.0 KB) - added by MikeHansenMe 5 years ago.
#21292
21319-remove.diff (983 bytes) - added by MikeHansenMe 5 years ago.
#21319
22229-remove.diff (1.4 KB) - added by MikeHansenMe 5 years ago.
#22229
27326-remove.diff (1.8 KB) - added by MikeHansenMe 5 years ago.
#27326
28786-remove.diff (1.5 KB) - added by MikeHansenMe 5 years ago.
#28786
29557-remove.diff (887 bytes) - added by MikeHansenMe 5 years ago.
#29557

Download all attachments as: .zip

Change History (71)

This ticket was mentioned in Slack in #core by boone. View the logs.


5 years ago

#2 @jorbin
5 years ago

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.

#3 @boonebgorges
5 years ago

+1 to all of that. Thanks, jorbin.

@MikeHansenMe
5 years ago

#4 @MikeHansenMe
5 years ago

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.

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

#5 @boonebgorges
5 years ago

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.

#6 @boonebgorges
5 years ago

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.

#7 @boonebgorges
5 years ago

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.

#8 @boonebgorges
5 years ago

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.

#9 @boonebgorges
5 years ago

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.

#10 @jorbin
5 years ago

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

#11 @jorbin
5 years ago

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

#12 @jorbin
5 years ago

In 30290:

Remove failing shortcode unit tests

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

see #30284

#13 @jorbin
5 years ago

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

This ticket was mentioned in Slack in #core by boone. View the logs.


5 years ago

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


5 years ago

#16 @miqrogroove
5 years ago

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

#17 @jorbin
5 years ago

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

#18 @miqrogroove
5 years ago

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

#19 @nacin
5 years ago

In 30448:

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

#20 @nacin
5 years ago

In 30451:

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

#21 @nacin
5 years ago

In 30453:

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

#22 @nacin
5 years ago

In 30454:

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

This ticket was mentioned in Slack in #core by boone. View the logs.


5 years ago

#24 @boonebgorges
5 years ago

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.

#25 @boonebgorges
5 years ago

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.

#26 @boonebgorges
5 years ago

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.

#27 @boonebgorges
5 years ago

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.

#28 @boonebgorges
5 years ago

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.

#29 @boonebgorges
5 years ago

In 30519:

Remove failing test in the hooks group.

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

Props MikeHansenMe.
See #30284.

#30 @boonebgorges
5 years ago

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.

#31 @boonebgorges
5 years ago

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.

#32 @boonebgorges
5 years ago

In 30522:

Remove failing test related to wp_unique_post_slug().

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

See #30284.

#33 @boonebgorges
5 years ago

In 30523:

Remove failing test related to wp_list_pages().

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

Props MikeHansenMe.
See #30284.

#34 @boonebgorges
5 years ago

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.

#35 @boonebgorges
5 years ago

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.

#36 @boonebgorges
5 years ago

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.

#37 @boonebgorges
5 years ago

In 30530:

Remove failing test_network_limit() XML-RPC test.

Test added as a patch on #21292.

Props MikeHansenMe.
See #30284.

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


5 years ago

#39 @boonebgorges
5 years ago

In 31253:

Remove skipped tests related to the proposed Export API.

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

See #30284.

#40 @boonebgorges
5 years ago

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.

#41 @boonebgorges
5 years ago

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.

#42 @boonebgorges
5 years ago

  • Milestone changed from Future Release to 4.2
  • Resolution set to fixed
  • Status changed from new to closed

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.

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by dd32. View the logs.


3 years ago

#45 @johnbillion
2 years ago

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

#46 @johnbillion
2 years ago

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

#47 @johnbillion
2 years ago

In 40452:

Remove failing test in the hooks group.

See #30284.

Merges [30519] to the 4.0 branch.

See #40463

#48 @johnbillion
2 years ago

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

#49 @johnbillion
2 years ago

In 40454:

Remove failing is_textdomain_loaded() test.

See #30284.

Merges [30521] to the 4.0 branch.

See #40463

#50 @johnbillion
2 years ago

In 40455:

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

See #30284.

Merges [30513] to the 4.0 branch.

See #40463

#51 @johnbillion
2 years ago

In 41304:

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

See #30284.

Merges [30285] to the 4.0 branch.

Note: See TracTickets for help on using tickets.