WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 4 months ago

#40910 new defect (bug)

Limit writable directories required by WordPress unit test suite

Reported by: danielbachhuber Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: needs-patch
Focuses: Cc:
PR Number:

Description

Some environments only permit writing to specific directories. But, the WordPress test suite currently assumes it can write to a variety of directories.

Based on searches for file_put_contents(), copy(), and symlink(), here are the directories I found:

  • tests/phpunit/data
    • .trac-ticket-cache.* is created to store a cache of Trac ticket numbers.
  • tests/phpunit/build/logs/
    • junit.xml is created with the results of the test run.
  • tests/qunit/fixtures
    • wp-api-generated.js is generated by PHPUnit for use in QUnit tests.
  • /tmp/
    • Variety of files are copied out of phpunit/data into /tmp/ for test-specific transformation.
  • wp-content/themes
    • theme-file-parent and theme-file-child are symlinked from the PHPUnit data directory into the theme directory
  • wp-content/uploads
    • Attachments are copied

Generally, in write-restricted environments, the test suite fails when it tries to write to directories it doesn't have access to. More directories could be found by running the test suite against a write-restricted environment.

To better ensure WordPress compatibility with a variety of platforms, we should ensure these file write requirements are compatible with limited write directories. Here are some strategies we can use:

  • Properly use wp_tempnam() instead of hardcoding /tmp/ paths.
  • Permit some write directories to be configurable with a constant or environment variable.
  • Failing gracefully, in certain cases, when a file can't be written.

cc @mikeschroder @octalmage

Change History (11)

#1 follow-up: @jnylen0
2 years ago

For your first three examples, at least, shouldn't we always be able to assume that anything under the source checkout of WordPress is writable?

Otherwise, this implies that two different users did the source checkout vs. the test run, which seems like a very odd situation (a non-writable source checkout is probably not all that useful for development-related tasks like running the test suite).

#2 in reply to: ↑ 1 @danielbachhuber
2 years ago

Replying to jnylen0:

For your first three examples, at least, shouldn't we always be able to assume that anything under the source checkout of WordPress is writable?

Not necessarily, if we want to run the test suite against production-esque environments for a variety of hosting companies, and report the results back to WordPress.org.

For instance, in Git mode, Pantheon delivers the source files to the environment via Git, and only the uploads directory is writable.

Similarly, other hosts restrict write access to the wp-content directory.

#3 follow-up: @jnylen0
2 years ago

.trac-ticket-cache.* is created to store a cache of Trac ticket numbers.

I really, really wish we would just get rid of these - our test suite should not be calling out to our issue tracker.

wp-api-generated.js is generated by PHPUnit for use in QUnit tests.

It should be fine to ignore a failure to update this file, as wp-api-generated.js is checked into source control and is not usually updated.

#4 in reply to: ↑ description @netweb
2 years ago

Replying to danielbachhuber:

  • Properly use wp_tempnam() instead of hardcoding /tmp/ paths.

There's a patch for this in #39975, it's using get_temp_dir() instead of wp_tempnam()

#5 in reply to: ↑ 3 ; follow-up: @dd32
2 years ago

Replying to jnylen0:

.trac-ticket-cache.* is created to store a cache of Trac ticket numbers.

I really, really wish we would just get rid of these - our test suite should not be calling out to our issue tracker.

Originally I believe this was for skipping known-failing test cases for open tickets, although I think it may have been repurposed over the years (I'm not quite sure). Since we now only commit tests that are expected to be operational that functionality can probably be removed from the test runner..

#6 in reply to: ↑ 5 @netweb
2 years ago

Replying to dd32:

Replying to jnylen0:

.trac-ticket-cache.* is created to store a cache of Trac ticket numbers.

I really, really wish we would just get rid of these - our test suite should not be calling out to our issue tracker.

Originally I believe this was for skipping known-failing test cases for open tickets, although I think it may have been repurposed over the years (I'm not quite sure).

Most recent work in this area was done in #30284

This ticket was mentioned in Slack in #hosting-community by timbutlerau. View the logs.


8 months ago

#8 @andraganescu
7 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Reading through #30284 it appears that the original intention was to remove all the .trac-ticket-cache.* but apparently it got omitted.

Indeed we should use wp_tempnam() instead of hardcoding /tmp/ paths and failing gracefully, seems a sensible idea.

Also, what do you mean by "permitting some write directories to be configurable with a constant or environment variable", I'm not sure I understand what this means. Do you have a specific case in mind for this?

#9 @andraganescu
7 months ago

  • Keywords good-first-bug added

This ticket was mentioned in Slack in #core by sergey. View the logs.


5 months ago

#11 @desrosj
4 months ago

  • Keywords good-first-bug removed
Note: See TracTickets for help on using tickets.