WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#41561 closed enhancement (fixed)

Remove usage of DOING_AJAX in the test suite

Reported by: johnbillion Owned by: johnbillion
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: needs-unit-tests
Focuses: Cc:

Description

In the test suite, several tests define DOING_AJAX to true. This effectively pollutes the tests which follow them, and prevent any other tests from operating in a non-Ajax state when they need to (for example, Tests_WP_Customize_Manager::test_not_doing_ajax()).

Instances of DOING_AJAX should be replaced by use of a filter, for example add_filter( 'wp_doing_ajax', '__return_true' ).

Attachments (3)

41561.diff (2.7 KB) - added by Mte90 3 years ago.
41561.2.diff (3.0 KB) - added by Mte90 3 years ago.
removed marktestskipped
41561.3.diff (883 bytes) - added by peterwilsoncc 3 years ago.

Download all attachments as: .zip

Change History (14)

#1 @johnbillion
3 years ago

Previously: #25669

@Mte90
3 years ago

#2 @Mte90
3 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

I was looking on simple tickets to start with unit tests so this was perfect.
I checked and with this changes there was no problems with the tests :-)

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


3 years ago

#4 @johnbillion
3 years ago

  • Keywords dev-feedback removed
  • Milestone changed from Awaiting Review to 4.9

@Mte90 Looks good!

  1. The test_not_doing_ajax() test can use add_filter( 'wp_doing_ajax', '__return_false' ) to force a non-Ajax request, and then remove the markTestSkipped call.
  2. Does this cover all instances of DOING_AJAX in the test suite?

#5 @Mte90
3 years ago

  1. I will do a new patch for that asap
  2. I searched in all the code about the use of the constant and replace with the function :-)

@Mte90
3 years ago

removed marktestskipped

#6 @johnbillion
3 years ago

  • Owner set to johnbillion
  • Status changed from new to reviewing

#7 @johnbillion
3 years ago

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

In 41293:

Build/Test tools: Remove usage of DOING_AJAX from the test suite, so all tests that expect either an Ajax request or a
non-Ajax request can operate without being skipped.

Props Mte90

Fixes #41561

#8 @matthias.thiel
3 years ago

I am not sure the current implementation resolves the issue properly. Adding a filter in the static setUpBeforeClass method will still exist in test cases that are run later. I advise putting the hook into the setUp method like the wp_die_ajax_handler hook.

I think this would also be more in line with the WP_UnitTestCase class as it backs up all hooks in the setUp method.

#9 @johnbillion
3 years ago

  • Keywords needs-unit-tests added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening as per comment above.

#10 @peterwilsoncc
3 years ago

41561.3.diff moves the filter from setUpBeforeClass() to setUp() in WP_Ajax_UnitTestCase. All other occurrences are in the correct location.

#11 @peterwilsoncc
3 years ago

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

In 41970:

Build/Test tools: Move wp_doing_ajax defintion from class setup to test setup.

Moves defintion of ajax request in WP_Ajax_UnitTestCase to setUp() method to account for hooks being reset as part of tearDown().

Props matthias.thiel for report.
Fixes #41561.

Note: See TracTickets for help on using tickets.