SimpleTest proper installation of test files and removal

Never noticed I am assuming because I had files install correctly once. I also added deletion of files and folder during un-install along with comments.

Upload test passes and after fresh install of SimpleTest (deleted files) it seems to work.

Sorry, could I please have some test instructions with this patch?

Should uninstall SimpleTest and make sure that the files/simpletest directory doesn't exists. (it won't remove them without patch, so you will have to do manually)

Install SimpleTest and make sure that it puts all the files there and upon uninstall it removes them.

Guess I should change back to RTBC?

I confirmed that the uninstall stuff does what's intended. I still don't quite grok the copy stuff. That probably means it needs to be better documented than "// Copy other test files for consistency." Is that overwriting the files in the files directory with whatever's in simpletest's files directory, or..?

Added extra comment.

Re-rolled.

Shoot. :( No longer applies. Sorry. :(

Re-rolled.

Attached patch correct typo :

+ // Unistall schema.

+    // If there are more files in the original directory then the files directory, copy files.

"then" is also a typo. It should be "than".

Forgot to change status.

Re-rolled...

ping...ping

Oops, I thought I committed this already!

Committed to HEAD. Thanks. :)

Bzzzt. Spoke too soon.

hook_uninstall() appears to be b0rked.

I get a bazillion of these:

# Warning: preg_match(): No ending delimiter '.' found in file_scan_directory() (line 1294 of /Applications/MAMP/htdocs/core/includes/file.inc).

And then:

# Warning: rmdir(sites/default/files/simpletest): Directory not empty in simpletest_uninstall() (line 112 of /Applications/MAMP/htdocs/core/modules/simpletest/simpletest.install).

And so does hook_install() for that matter:

Warning: preg_match(): Unknown modifier '-' in file_scan_directory() (line 1294 of /Applications/MAMP/htdocs/core/includes/file.inc).

x40 or so.

Not sure if this patch needs work or if it's just waiting on #74645: modify file_scan_directory to include a regex for the nomask. to get re-committed, but it's definitely NOT working, so rolled it back. Sorry.

issue with core.

Good comments; even I can understand the patch now. ;)

I assume there is no way for the destination to contain files /not/ in the original path, which would make the file-count comparison unreliable. Common sense says we can always assume that, but I'm mentioning it just in case.

We should be able to assume that, but otherwise it could do a comparison of the files themselves.

This looks good. However, I'd add one sentence to + // If there are more files in the original directory than the files directory, copy files. explain _when_ or _why_ this can happen.

This behaves like the "Clean environment" button. More for automatically fixing any issues with the test environment.

The uninstall behavior is technically a bug since it should not leave all the test files behind.

+    // If there are more files in the original directory than the files
+    // directory, copy files. This can occur when test files are deleted from
+    // the SimpleTest directory. It servers a convenience to developers so that
+    // they can get the files back easily.

"servers" sounds like a typo.

So, right now, according to the documentation, there are 3 directories: the original directory, the files directory and the SimpleTest directory. I initially thought that the original directory was the SimpleTest directory but that doesn't make sense when I read the code comments. It is still not clear how files can disappear from the original directory?

"servers" => "serves"

The SimpleTest directory stuff was a messup. Not sure how I managed that. :)

This looks good to me -- it is reasonably clear now. Let's see what others have to say.

I like the comment.

Great. Thanks. Committed! Next patch? :)