WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#37523 closed enhancement (fixed)

Unit tests: Move classes out of includes/functions.php into their own file

Reported by: Frank Klein Owned by: boonebgorges
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.6
Component: Build/Test Tools Keywords: good-first-bug has-patch needs-testing
Focuses: Cc:

Description

The includes/functions.php file includes two classes, Basic_Object and Basic_Subclass. The attached patch moves these functions into their own file, and includes them in tests/basic.php.

Attachments (3)

37523.diff (3.1 KB) - added by Frank Klein 3 years ago.
37523.patch (2.0 KB) - added by DylanAuty 3 years ago.
Revised Patch based on suggestions/solutions offered.
37523.2.patch (2.0 KB) - added by DylanAuty 3 years ago.
New Revision of the previous patch file I submitted. With the changes suggested by @Frank Klein. I moved the including of the classes to the bootstrap.php file instead. Below the inclusion of the funtions.php file as no reference is made to the classes up to this point. Did not test this, however it seems like the revision would run as expected

Download all attachments as: .zip

Change History (14)

@Frank Klein
3 years ago

#1 @boonebgorges
3 years ago

  • Milestone changed from Awaiting Review to 4.7
  • Owner set to boonebgorges
  • Status changed from new to assigned

@Frank Klein Thanks for the patch!

#2 @boonebgorges
3 years ago

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

In 38285:

Tests: Move some utility classes to their own files.

Props Frank Klein.
Fixes #37523.

#3 follow-up: @TimothyBlynJacobs
3 years ago

This broke unit tests that I have that use the Basic_Object class in a test since they are now only loaded in that test file instead of the functions file. Could the require calls be moved to the functions file so those classes are always available?

#4 in reply to: ↑ 3 @boonebgorges
3 years ago

  • Keywords needs-patch added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to TimothyBlynJacobs:

This broke unit tests that I have that use the Basic_Object class in a test since they are now only loaded in that test file instead of the functions file. Could the require calls be moved to the functions file so those classes are always available?

Yes.

#5 @boonebgorges
3 years ago

  • Keywords good-first-bug added

@DylanAuty
3 years ago

Revised Patch based on suggestions/solutions offered.

#6 @DylanAuty
3 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#7 follow-up: @Frank Klein
3 years ago

I think it would be cleaner to load the files during the bootstrap process, instead of including them in functions.php. Nothing in this file relies on the two classes, but the tests do.

There's a small typo in the patch. It's in the word UnitTests in the documentation header of class-basic-object.php.

@DylanAuty
3 years ago

New Revision of the previous patch file I submitted. With the changes suggested by @Frank Klein. I moved the including of the classes to the bootstrap.php file instead. Below the inclusion of the funtions.php file as no reference is made to the classes up to this point. Did not test this, however it seems like the revision would run as expected

#8 in reply to: ↑ 7 @DylanAuty
3 years ago

Replying to Frank Klein:

I think it would be cleaner to load the files during the bootstrap process, instead of including them in functions.php. Nothing in this file relies on the two classes, but the tests do.

There's a small typo in the patch. It's in the word UnitTests in the documentation header of class-basic-object.php.

Thanks for pointing that out. I have revised my patch and submitted it.

Apologies for the typo on that one.

#9 @boonebgorges
3 years ago

  • Status changed from reopened to assigned

Thanks for the patches. I'm inclined to leave the require_once statements in functions.php, given that:

  • our main goal is backward compatibility, and functions.php is where the classes were previously loaded in the call graph
  • none of this will matter once we autoload classes; see #36335

#10 @boonebgorges
3 years ago

In 38444:

Tests: Add docblocks for Basic_Object and Basic_Subclass classes.

Props DylanAuty.
See #37523.

#11 @boonebgorges
3 years ago

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

In 38445:

Tests: Require Basic_Object and Basic_Subclass files earlier in call stack.

This ensures compatibility with third-party tools using these classes
in their test suites, after [38285].

Props DylanAuty, Frank Klein, TimothyBlynJacobs.
Fixes #37523.

Note: See TracTickets for help on using tickets.