Opened 2 years ago
Closed 11 months ago
#57774 closed task (blessed) (fixed)
Add some initial PHPUnit tests for `WP_Filesystem_Direct`
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.5 | Priority: | normal |
Severity: | normal | Version: | 2.5 |
Component: | Build/Test Tools | Keywords: | has-patch has-unit-tests commit |
Focuses: | Cc: |
Description
This is a tests-only ticket.
WP_Filesystem_Direct
is, by far, the most used filesystem abstraction class.
While some PHPUnit tests exist for a WP_Filesystem_MockFS
class, this ticket aims to add PHPUnit tests for the WP_Filesystem_Direct
class itself.
The other filesystem abstraction classes, WP_Filesystem_FTPext
, WP_Filesystem_ftpsockets
and WP_Filesystem_SSH2
, do not have PHPUnit test runner configurations. While in future, we may reuse many of the same tests across filesystem abstraction classes, this ticket specifically targets WP_Filesystem_Direct
.
Change History (34)
This ticket was mentioned in βPR #4106 on βWordPress/wordpress-develop by β@costdev.
2 years ago
#1
- Keywords has-unit-tests added
#2
@
2 years ago
βPR 4106 is ready for review.
Line coverage: 74.56%
Branch coverage: 76.83%
To cover more lines/branches, source changes are needed to facilitate testing/fix bugs. This will be done in separate tickets.
#3
@
2 years ago
- Owner changed from costdev to hellofromTonya
- Status changed from assigned to reviewing
Self-assigning for review and commit consideration.
This ticket was mentioned in βSlack in #core by mukeshpanchal27. βView the logs.
2 years ago
#5
@
2 years ago
- Milestone changed from 6.2 to 6.3
This ticket was discussed in the bug scrub.
While this is a tests-only ticket and can be committed at any time, we're approaching RC1 and the PR is awaiting review, so I'm going to move this ticket to the 6.3 milestone.
Should it be reviewed and ready for committed prior to 6.2 RC1, feel free to bring it back into the 6.2 milestone for commit.
Additional props: @mukesh27
This ticket was mentioned in βSlack in #core by chaion07. βView the logs.
19 months ago
#8
@
19 months ago
- Milestone changed from 6.3 to Future Release
- Type changed from defect (bug) to enhancement
#9
@
18 months ago
- Milestone changed from Future Release to 6.4
Moving into the 6.4 milestone.
@costdev sorry for the big delay. I'll get your PR reviewed and then committed.
#10
@
17 months ago
- Keywords needs-refresh added
The patch needs to be refreshed, most likely the part in phpcs.xml.dist is not needed anymore, see:
βhttps://github.com/WordPress/wordpress-develop/commit/60d2d8ac75c4350a45e4c6970c7d0295b757f1f0
Great job, btw. Let's try to make it into the current milestone :)
#11
@
17 months ago
- Keywords needs-refresh removed
Thanks @oglekler!
βPR 4106 has now been rebased on trunk
and the conflict with phpcs.xml.dist
resolved.
This ticket was mentioned in βSlack in #core by oglekler. βView the logs.
17 months ago
#13
@
17 months ago
This ticket was discussed during bug scrub.
@hellofromTonya can you please review this PR?
Add props to @mukesh27
This ticket was mentioned in βSlack in #core by oglekler. βView the logs.
17 months ago
#15
@
17 months ago
As this ticket is for adding automated tests, it is not bound to the 6.4 release milestones and can be committed at any time before the 6.4 Dry Cycle starts.
#16
@
17 months ago
- Component changed from Filesystem API to Build/Test Tools
To avoid confusion for 6.4 triagers during the beta cycle, I changed the component to Build/Test as this ticket tracks only tests.
#17
@
16 months ago
- Milestone changed from 6.4 to 6.5
While this ticket is not bound to a major release's schedule, tomorrow RC1 ships and trunk
will be branched to open it for 6.5-alpha. Thus, given the short time remaining in 6.4, moving this to 6.5.
What's left? Code review and then commit. If that can get done during 6.4 RC cycle, could consider backporting the tests to the 6.4-branch.
#19
@
12 months ago
Hey @costdev, we previously chatted about the patch's readiness. Fast wind to today, is it ready for code and commit review?
#20
@
12 months ago
Hey @hellofromTonya! Yes I believe the PR should be ready for code and commit review.
β@swissspidy commented on βPR #4106:
11 months ago
#23
#24
@
11 months ago
- Resolution fixed deleted
- Status changed from closed to reopened
I've noticed these tests are causing a bunch of failures on most of the βhosting providers running the test suite. Common issues appear to be disallowed operations and attempting to remove directories that are not empty.
Unfortunately there's no way of testing PRs against the hosting providers so it wasn't possible to preempt the issue but it's probably worth reverting the change for now as it's late in the release cycle and coming back to this in the 6.6 release cycle.
#25
@
11 months ago
Thanks @peterwilsoncc, I was actually about to reopen or open a new one myself as weβre actually working on the hosting tests this weekend and noticed the failures.
Not sure about reverting as theyβre just tests & not functionality, and also already in the 6.5 branch I think.
Would be good to first analyze the common failures a bit more to find the root cause. Iβd imagine we could also just skip the whole tests if lacking permissions for example.
#26
@
11 months ago
@swissspidy I've asked in the β#hosting slack channel whether the hosting tests always run on trunk or on the branch for backport commits.
If they always run on trunk then I agree that there's not much point in reverting. If they run on the backport branch then it might be worth reverting from the 6.5 branch so the reports for backports are at least clear.
Iβd imagine we could also just skip the whole tests if lacking permissions for example.
I think that works.
#27
@
11 months ago
I agree that we should establish the exact cause of the errors first.
For example, maybe the test runner isn't running with FS_METHOD
set to direct
.
So far, I see "Operation not permitted" for chown/chgrp()
, and "Directory not empty" for rmdir()
.
The first seems clear as a permissions issue, but the error occurs in the actual class file, not the test file. That points to a legitimate error when trying to use the direct class in that environment.
For the "Directory not empty" failure, while I could theorise about possible causes, I'd be very curious to see further details specifically from those environments.
This ticket was mentioned in βPR #6286 on βWordPress/wordpress-develop by β@swissspidy.
11 months ago
#28
Trac ticket: https://core.trac.wordpress.org/ticket/57774
#29
@
11 months ago
I was able to reproduce the issues locally as well.
Operation not permitted
The issue seems to be the line:
$expected_group = $this->is_windows() ? $original_owner : $original_owner + 1;
$original_owner
is the result of fileowner()
, which is the user ID of the current owner).
On my system there is no such user with ID+1, hence the operation is not permitted.
I think that's an easy thing to solve, just don't use another user ID.
Directory not empty
The tests seem to pass when run in isolation, so it's probably some missing cleanup elsewhere.
In my quick testing, fixing the Operation not permitted
issue actually seemed to solve these ones here. So two birds with one stone.
Here's a PR: βhttps://github.com/WordPress/wordpress-develop/pull/6286
If it passes we can commit this and then see how the hosting tests are afterwards.
#30
@
11 months ago
There's a third issue I just noticed:
Tests_Filesystem_WpFilesystemDirect_Mkdir::test_should_set_chmod The permissions are incorrect. Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -'0644' +'0755' /home/wptestrunner/site/public_html/wp-test-runner/tests/phpunit/tests/filesystem/wpFilesystemDirect/mkdir.php:133
See βhttps://make.wordpress.org/hosting/test-results/r57848/wpmudevbot-r57848/
Maybe that host sets FS_CHMOD_DIR
to something custom?
After βmy PR, this test is the last one with an is_windows()
check. Would be good if we could get rid of this env-specific check and maybe rely on comparing with FS_CHMOD_DIR
instead.
β@swissspidy commented on βPR #6286:
11 months ago
#32
Committed in https://core.trac.wordpress.org/changeset/57849
This adds some initial tests for the
WP_Filesystem_Direct
filesystem abstraction class.These tests do not aim to cover all paths, but to add line/branch coverage where source changes are not needed to facilitate testing or resolve bugs. The intention is to make it easier to know how to hit a line/branch when trying to cover paths in future.
This also introduces the
WP_Filesystem_Direct_UnitTestCase
class, which contains shared properties,set_up()/tear_down()
, helpers and common data providers for the tests.To allow constants to be set without affecting other tests in the test suite, some of these tests use the
@runInSeparateProcess
annotation β[Ref]. To prevent an exception forSerialization of 'Closure' is not allowed
, the@preserveGlobalState disabled
annotation is also used β[Ref].Tests exist for the following methods:
__construct()
get_contents()
get_contents_array()
put_contents()
cwd()
chdir()
chgrp()
chmod()
chown()
getchmod()
copy()
move()
delete()
exists()
is_file()
is_dir()
is_readable()
is_writable()
atime()
mtime()
size()
touch()
mkdir()
rmdir()
dirlist()
Trac ticket: https://core.trac.wordpress.org/ticket/57774