Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Correct unit tests when running WP 5.7 #774

Merged
merged 6 commits into from Apr 21, 2021
Merged

FIX: Correct unit tests when running WP 5.7 #774

merged 6 commits into from Apr 21, 2021

Conversation

nielslange
Copy link
Member

@nielslange nielslange commented Apr 1, 2021

Fixes #458

Changes

Correct two unit tests that are causing an error when running under WP 5.7.

Steps to test

  1. Check out this PR
  2. Run phpunit in the plugin directory

Notes

Testing this PR requires a temporary WordPress installation. A temporary WordPress installation can be created with the following steps:

  1. Install MariaDB via brew install mariadb
  2. Start MariaDB as service via brew services start mariadb
  3. Go to plugin directory
  4. Run ./bin/install-wp-tests.sh co_authors_plus root root

Please note that this PR only resolves the problem that appears when using WordPress 5.7, the latest available version. It does not resolve the theme-related failed test case when using WordPress 5.3.

Please also note that the previous featured image was an artificial file and not a real image file. This caused a problem in the Twenty Twenty-One theme as this artificial file does not have any metadata such as a height or a width. Therefore, I've replaced that artificial file with a real image dummy file.

@nielslange nielslange changed the title 🐛 FIX: Correct unit tests when running WP 5.7 FIX: Correct unit tests when running WP 5.7 Apr 7, 2021
@nielslange nielslange marked this pull request as ready for review April 9, 2021 01:53
htdat
htdat previously approved these changes Apr 9, 2021
Copy link
Member

@htdat htdat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job here!

If possible, it would be helpful if there was an explanation why this PR changes the way of creating attachments.

As I understand, the old way of creating attachments without a real file will not create metadata for this attachment. And then the test (with WP 5.7) failed in line 136 of TwentyOne theme https://github.com/WordPress/WordPress/blob/5.7/wp-content/themes/twentytwentyone/inc/template-functions.php#L435-L436 as $meta here will be false:

		$meta = wp_get_attachment_metadata( $attachment->ID );
		if ( $meta['width'] && $meta['height'] ) {

tests/test-coauthors-guest-authors.php Outdated Show resolved Hide resolved
@htdat
Copy link
Member

htdat commented Apr 9, 2021

Also, it would be nice to create a separate PR to add PHP 7.3 and PHP 7.4 to .travis.yml as currently the plugin is not running tests for these PHP versions.

# aliased to a recent 7.x version
- php: '7.0'
env: WP_VERSION=5.6.1
- php: '7.0'
env: WP_VERSION=latest
- php: '7.1'
env: WP_VERSION=5.6.1
- php: '7.1'
env: WP_VERSION=latest

@GaryJones
Copy link
Contributor

Could also add TravisCI jobs for PHP 8.0 and nightly (allow_failure) as well

@nielslange
Copy link
Member Author

Also, it would be nice to create a separate PR to add PHP 7.3 and PHP 7.4 to .travis.yml as currently the plugin is not running tests for these PHP versions.

Could also add TravisCI jobs for PHP 8.0 and nightly (allow_failure) as well

@htdat & @GaryJones I created #777 to switch from Travis CI to Circle CI.

@nielslange
Copy link
Member Author

@rebeccahum and @GaryJones Can I have your thoughts regarding the failed Travis CI tests as seen on https://travis-ci.org/github/Automattic/Co-Authors-Plus/builds/766742529? Earlier today, I replaced assertContains() with assertStringContainsString(), as assertContains() is depricated and will be removed with PHPUnit 9. However, this repo was still using PHPUnit 5, which does not support assertStringContainsString() yet. I bumped the PHPUnit version from 5 to 7, to notice that PHPUnit 7 requires PHP 7.1, while the minimum required version to run WordPress is PHP 5.6.

Given that PHP 7.4 or greater is the official recommended version based on https://wordpress.org/about/requirements/, I tend run the PHPUnit tests only against PHP 7.1 or greater. I'm keen to hear your thoughts on this matter.

@GaryJones
Copy link
Contributor

If the plugin is still meant to run on PHP 5.6, then you can use PHPUnit 7.5, and ignore the platform requirements when installing Composer dependencies in the Travis config.

See https://github.com/Yoast/wordpress-seo/blob/ba38278c1d94c09d11f92ed6646930c72e3e4810/.travis.yml#L136-L154 as an example.

@nielslange
Copy link
Member Author

Thanks for your thoughts and pointing out Yoast's example, @GaryJones.

@nielslange
Copy link
Member Author

@rebeccahum Could you look up this change, to check if it's in a shape to get merged, to reduce the noise in the unit tests?

@rebeccahum rebeccahum merged commit be19758 into master Apr 21, 2021
@rebeccahum rebeccahum deleted the fix/#458 branch April 21, 2021 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests failing with PHP 7
4 participants