Adds enum unit tests #26
Conversation
There was a problem hiding this comment.
@JasonTheAdams Thanks, this looks great!
Just a few small details I think we may want to revise. But overall almost good to merge.
Nit-pick: Could you avoid prefixing PR branches with feature/? The idea (relevant e.g. for php-lint.yml) is that that prefix should only be used for feature branches - i.e. branches where a new larger feature is being developed across several PRs separate from trunk.
.github/workflows/unit-tests.yml
Outdated
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| php-version: ['7.4', '8.4'] |
There was a problem hiding this comment.
This is a pragmatic start, although I'm wondering whether we should at least cover one in between, e.g. 8.0?
WordPress even tests all versions, and while that might be excessive for this project, maybe there is a middle ground?
There was a problem hiding this comment.
I've never personally understood the reason for that. In my mind if it's compatible with 8.4 then it's compatible with each step between. I guess an argument could be made that it will show at exactly which version something is breaking... but that feels like a moot point if the goal is to be compatible all the way up to 8.4. We've done it this way in a handful of our plugins for years with no noticeable downside.
That said, it's really not a big deal, so I'm happy to add it if you still feel it's best. 😄
There was a problem hiding this comment.
Usually what you're saying is right. But there can always be nuances between versions in little things we wouldn't foresee, and for a project as big as WordPress Core, I think it's worth going for the always - in other words, test with every PHP version to do the closest possible in detecting any possible errors, even if unlikely.
This SDK is obviously not the same reach/scale, so no need to do it here, but I do think one version in between would be helpful, also to get a better idea when/where something broke. This will make finding the root and fixing the bug faster for us too.
There was a problem hiding this comment.
Sound reasoning. Thanks for walking me through that. I've resolved this by adding 8.0 in d5bdb61
| @@ -0,0 +1,17 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Is it a common pattern in PHP projects to have mock files or utility classes for testing colocated between all the actual tests?
If so, I'd be okay with that, but based on what I'm used to from WordPress, I'd consider this a bit messy. An alternative would be to separate tests/unit/tests from tests/unit/lib or something like that. Then the first would only contain actual tests.
Curious what you think. Have you seen the pattern here in other popular PHP projects?
There was a problem hiding this comment.
Yeah, I thought about that. I actually meant to circle back, as I don't like colocation because it breaks the 1-to-1 pattern of src to the unit tests. The pattern I've liked is tests/mocks, so I'll start with that.
There was a problem hiding this comment.
Tentatively resolved in fbc4501. Open to your thoughts!
|
I resolved most of your feedback, @felixarntz, and left some comments! Funny on the branch naming. I think of feature branches as a unit of work introducing a feature or portion thereof. I've used epic branches ( |
There was a problem hiding this comment.
@JasonTheAdams Thanks, tentatively approving this with minor feedback, see below.
I think breaking out the mocks into their own directory makes sense. In the future we may also need other infra test classes that aren't mocks, but no need to cater for that now, as we don't know.
My one suggestion would be to organize the nesting of the directories as follows:
tests/unit/teststests/unit/mocks
The reason for that is that unit tests are one type of tests, and there could be others we may eventually implement. Currently, tests/mocks is outside of tests/unit, although it also relates exclusively to unit tests.
Alternatively, if we prefer not to have the extra nesting, we could for now decide to consider the entire tests directory exclusively for unit tests. In that case, I'd suggest to give the tests/unit directory another name, as the overarching tests directory as a whole would be for unit tests overall, and what's called unit here would contain the actual tests. Let me know what you prefer.
Co-authored-by: Felix Arntz <[email protected]>
|
After a quick DM chat with @JasonTheAdams, we aligned on keeping the tests organized as currently is - works for now. 👍 |
Depends on #25
This adds the PHPUnit infrastructure and the unit tests for the enums from #25. It also adds a Github workflow to run the unit tests for pull requests and pushes.