This issue forked off of #203204: Uploaded files have the permissions set to 600 which is now postponed waiting on this.

Here's a file testing clean up patch that:

  • Adds a test for--the currently untested--file_save_upload().
  • Creates a file_test.module to serve as a target for the uploads.
  • Adds a new FileTestCase class which serves as a base for the other test cases. It adds some helper functions and file specific assertions.
  • Splits up the file move/copy/delete function tests into their own classes to make it easier to just test a specific function. I think it'll also encourage more separate tests for each function.
  • Adds a chmod check to file_copy/move to validate that the expected change sticks.
CommentFileSizeAuthor
#7 drupal-file-test-cleanup-310358-7.patch44.37 KBwebchick
#6 file_test_cleanup.patch29.41 KBdrewish
#5 file_test_cleanup.patch27.38 KBdrewish
#1 file_test_cleanup.patch22.13 KBdrewish
file_test_cleanup.patch21.97 KBdrewish

Comments

drewish’s picture

drewish commented
StatusFileSize
new file_test_cleanup.patch22.13 KB

here's a couple of cleanups suggested by cwgordon7

cwgordon7’s picture

cwgordon7 commented

This looks pretty good, much cleaner code overall. Small nitpick:

"File permissions set correctly." should probably be 'File permissions set correctly.' Same goes for the message below. Other than that, I can't actually find any problems with this patch, so it'll be pretty much rtbc then.

catch’s picture

catch
he/him
Primary language English
commented

Ran the tests and they go fine. For really nitpicky stuff, a few inline comments are missing full stops, it'd be nice if drupalwebtestcase handled the drupal_get_messages() but that's a different issue to this one. Some of the functions are missing phpdoc testFileDelete_Directory should probably be testFileDeleteDirectory unless there's a compelling reason why not. Looks good overall though.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Needs work

Nice. This looks like a good clean-up.

While we're cleaning everything else up, can we please slap some one-liner PHPDoc on each of these test functions? I remember asking for that back when this patch originally got committed but it was late and I must've missed that they weren't there.

+  function assertPermissions($filepath, $expected_mode, $message = NULL) {

Could we please name that assertFilePermissions() or similar? To distinguish it from the user kind of permissions? I realize it's scoped with a class called FileTestCase, but even still, either could apply.

Also, $expected_mode doesn't match $mode in the PHPDoc above it.

Full stops / periods would be nice, as they'll save me time when committing. I agree with catch that testFileDelete_Directory is unconventional, and should be mooshed together since that's the standard, for better or worse.

I would normally say move the drupal_set_message() to drupal_web_test_case.php as part of another patch, but it's a really easy change, so let's just do it here as well.

Once these changes are made, I have no problem committing this.

drewish’s picture

drewish commented
Status: Needs work » Needs review
StatusFileSize
new file_test_cleanup.patch27.38 KB

I think the whole smooshed naming of functions standard or not is totally stupid. testFileDelete_Directory was named specifically to indicate which function is being tested and what specific aspect is being tested. Since I'm breaking it out to one target function per class I'll just name the test cases after the aspect that they're checking.

drewish’s picture

drewish commented
StatusFileSize
new file_test_cleanup.patch29.41 KB

whoops, that last patch was missing the file_test.* files

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Fixed
StatusFileSize
new drupal-file-test-cleanup-310358-7.patch44.37 KB 
+    drupal_get_messages();

Added a comment above that line to explain it.

+    // Replace this once the hook_file patch gets committed in 2010.

Removed the snark. :P

Also fixed a bunch of minor stuff like single/double quotes, re-wording some comments, concatenation spacing, capitalization, etc. since this /was/ a "clean-up" patch, after all. Reading over all the tests in depth made me once again very grateful that we have these tests... there is a LOT of tweaky stuff in there that would take eons to test manually. Thanks, drewish & co!

Here's the version I committed. Thanks!

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.