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

Update integration tests: #2503

Merged
merged 5 commits into from
Oct 1, 2021
Merged

Update integration tests: #2503

merged 5 commits into from
Oct 1, 2021

Conversation

nlemoine
Copy link
Member

@nlemoine nlemoine commented Sep 28, 2021

Timber now officially supports PHP 8.0/8.1 🎉
(I mean if @jarednova is ok with this 😅)

PR summary:

Side notes:

  • PHPUnit has been removed as a dependency to let yoast/phpunit-polyfills handle the PHPUnit version to use as advised in link above
  • You can now use PHPUnit ^9.0 assertions
  • If you need to run tests on an older WP version (< 5.9), use composer req phpunit/phpunit:^7.5 -W --dev but don't commit changes made to composer.json
  • When installing test suite, always use a branch (5.8, not latest, not 5.7.1): bash bin/install-wp-tests.sh wordpress_test <db_user> <db_pass> <db_host> 5.8
  • Read the WordPress Core blog post above for further details

@coveralls
Copy link

coveralls commented Sep 28, 2021

Coverage Status

Coverage increased (+0.05%) to 89.997% when pulling 32e2c45 on 2.x-phpunit into ab7eb4b on 2.x.

- Update PHPUnit 🎉 thanks to https://make.wordpress.org/core/2021/09/27/changes-to-the-wordpress-core-php-test-suite/
- Update/fix tests
- Test Timber on PHP 8.0 🎉
gchtr
gchtr previously approved these changes Sep 30, 2021
Copy link
Member

@gchtr gchtr left a comment

Choose a reason for hiding this comment

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

Wow, Nico, this is amazing! 🙌🚀 I read that post and thought that is going to be quite some work. And you did it! Thank you so much for putting in the effort.

The only thing that came to my mind is that the latest keyword in https://github.com/timber/timber/blob/2.x/bin/install-wp-tests.sh might not work either. But it doesn’t look like they changed it in https://github.com/wp-cli/scaffold-command/blob/master/templates/install-wp-tests.sh either, yet.

And I wonder whether we need to update https://github.com/timber/timber/blob/2.x/docs/v2/guides/testing.md. Maybe @jarednova could have a look at that.

Both comments could be handled in separate pull requests and shouldn’t hold us back from merging this in so you can continue working, Nico :).

@nlemoine
Copy link
Member Author

nlemoine commented Sep 30, 2021

thought that is going to be quite some work

It was not that much, the only thing that took me while to understand is that the polyfilled test suite is only included in branches, not tags...

The only thing that came to my mind is that the latest keyword

This is only temporary, the time that scaffold tests are adapted to the new test suite. We could also modify install-wp-tests.sh so latest matches the latest branch, not tag.

But I think this is fine for now, I'll update these tests when a new WP version comes out or when scaffolded are updates.

I wonder whether we need to update

I'm pretty sure they should be :) I don't install VVV anymore and it works fine if you have everything setup.

And maybe in the future, switch to https://github.com/Yoast/wp-test-utils which eases tests a lot. And split real unit tests from integration tests to speed thing up. But one thing at a time :)

- Mark PHP 8.0 as non experimental
- Add PHP 8.1 as non experimental
- Fix PHP 8.0 tests
- Fix PHP 8.1 tests
@nlemoine
Copy link
Member Author

nlemoine commented Sep 30, 2021

Since PHP 8.1 is out soon... I added some fixes so everything passes tests.

Just a tiny question, I had trouble with image resizing when Imagick is available and crop is set to 'default'. What does 'default' means when cropping? I found nothing about it: https://github.com/timber/timber/blob/master/lib/Image/Operation/Resize.php#L119-L149

So I supposed default means no crop. Tests pass but I don't know resizing part well enough to see if there can be more consequences to this.

@jarednova
Copy link
Member

So this appears to be a leftover highly opinionated artifact of what I thought a good "default" crop should be circa 2015. The key lines are here ...

https://github.com/timber/timber/blob/master/lib/Image/Operation/Resize.php#L116-L117

... where it centers on the X axis and then 1/6 from the top on the Y axis. A perfectly sensible crop (this would be good for cropping to where faces generally are in photos) — but I can see why we'd want to make that controllable at the least. I'm going to say that for now, rearchitecting crops should be a 2.x future task.

I'll review this over the next couple days, formal PHP 8 support is great!!!

@jarednova
Copy link
Member

@nlemoine great work! as @gchtr said: thought that is going to be quite some work!!!! I'm sure it was.

Thanks for linking to the WP.org blog post, that answered all the big Qs I had in my review.

As @gchtr notes, this is a good time to get to those testing.md docs, or at least not tie them so directly to VVV. I've now got an Apple M1 that I've been meaning to get things set up with. What's your basic test setup? Docker? Or something else?

@nlemoine nlemoine merged commit 1d10cce into 2.x Oct 1, 2021
@nlemoine
Copy link
Member Author

nlemoine commented Oct 1, 2021

@jarednova I'm still on a mid-2012 macbook pro (non retina version)... Because this is the last one where you can still easily replace/upgrade your hard drive (I replaced the optical drive with a 2To SSD drive), memory and battery. It's running pretty good but it's getting white hairs and I try to avoid VM when I can. So basically, my setup is only brew install php && brew install mariadb.

The ideal way to run tests is act but it's not supporting services yet. Once it does, running tests will be quite easy and more importantly, we won't have to handle local env and Github Actions env.

@jarednova jarednova deleted the 2.x-phpunit branch October 1, 2021 12:05
@jarednova
Copy link
Member

Oh wow, you're going old school! Thanks for the links to act — it's great to have a way to execute those actions locally. I can't tell you how many hours I've spent pushing Travis commits just to see how it would use the build script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants