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

Tests: Require wporg-mu-plugins to run tests. #142

Merged
merged 3 commits into from
May 11, 2023

Conversation

iandunn
Copy link
Member

@iandunn iandunn commented Apr 28, 2023

Fixes #140

This requires wporg-mu-plugins to be installed because, without it, tests will fail incorrectly. Instructions for setting it up are in README.md, so I think it's safe to assume that it should exist.

If folks think it might trip people up, though, we could output an error message instead of letting it fatal. That could point people in the right direction if they're not sure why it's failing.

@iandunn iandunn self-assigned this Apr 28, 2023
@iandunn iandunn requested a review from dd32 April 28, 2023 20:41
@iandunn iandunn added this to the MVP milestone Apr 28, 2023
@iandunn iandunn marked this pull request as draft May 1, 2023 15:36
@iandunn
Copy link
Member Author

iandunn commented May 1, 2023

Converting back to draft while I look into action failures. Looks like a difference between the wp-env environment and mine.

@iandunn iandunn force-pushed the require-wporg-mu-plugins-for-tests branch from acf1511 to 45cf207 Compare May 1, 2023 17:18
Without it, tests will fail incorrectly in local environments.
@iandunn iandunn force-pushed the require-wporg-mu-plugins-for-tests branch from 45cf207 to de75c8d Compare May 1, 2023 17:20
@iandunn
Copy link
Member Author

iandunn commented May 1, 2023

Ok, I think de75c8d is a better approach. I also needed to move my wporg-mu-plugins checkout to the new path in the readme, so that it matches wp-env.

I noticed some other things that could be improved about the setup to make local and wp-env environments match, and logged those in #143.

@iandunn iandunn marked this pull request as ready for review May 1, 2023 17:26
@dd32
Copy link
Member

dd32 commented May 2, 2023

Personally, I don't want to support multiple ways of setting up local environments. wp-env has it's problems, but it's intentionally biased and we're reliant upon that. I'm in favour of removing the other steps from the readme and putting it into an errata or other document instead as an alternative setup options.

Looking at this diff, there's two things that jump out at me:

  1. The additional require shouldn't be required, as it's an mu-plugin it should be loaded as part of the WP install that the wp-phpunit requires
  2. again.. The existing requires of the plugin shouldn't be required, as the plugin should be loaded on the WP install.

But that being said, this has zero impact upon the existing wp-env usage, so I've got nothing against adding it, only against trying to support multiple environment types.

@StevenDufresne
Copy link
Contributor

I would also vote we treat this the same way we do with the other themes and install it via composer. Refs: wporg-main-2022:composer.json, wporg-main-2022:0-sandbox.php

@iandunn
Copy link
Member Author

iandunn commented May 2, 2023

I'm In favor of moving those steps into an alternative setup section, I've added that to #143 👍🏻

I think installing wporg-mu-plugins via composer.json is a good idea too, I'll add that to this PR 👍🏻

I also like the idea of a single environment in theory, and I've tried switching to wp-env and other Docker-based envs several times in the past. Each time, though, I've found that it doesn't work well for me personally, and it's just simpler to run nginx locally.

I'm happy to work around it though if you all would prefer to keep things as they are.

@iandunn
Copy link
Member Author

iandunn commented May 10, 2023

I added the Composer checkout in 5a36207, but GitHub Actions have a partial outage right now. The tests pass in my local env, but I'm going to wait until that's resolved to make sure they pass here too.

I'm planning to merge if they are, but let me know if either of you would like to discuss something different.

@iandunn iandunn merged commit 12a8458 into trunk May 11, 2023
@iandunn iandunn deleted the require-wporg-mu-plugins-for-tests branch May 11, 2023 00:24
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.

PHPUnit tests failing
3 participants