Store file_test.module's values in variables rather than globals

humm... can we hold off on this? i'm working on some code to refactor the whole way that those values are passed around. i was going to merge it in with #238299: file_create_url should return valid URLs but since this is here it'd probably be better as it's own patch.

okay so here's the patch, it removes the global variables which don't have the same context as the unit tests and uses variables which are shared between the tests and the module under test.

can you give this a try?

fixing some white space issues...

bumping this over to the test queue. i'd like to get this so that #276280: Tests needed: file.inc can use it as well.

Instead of

function file_test_file_insert(&$file) {
  $a = func_get_args();
  _file_test_set_call('insert', $a)

I think the following is easier to follow:

function file_test_file_insert(&$file) {
  _file_test_set_call('insert', array($file))

A few comments outside the scope of this bug - just ignore them if you think they are inappropriate:

• file_test_file_download(&$file) - in the documentation, this is $file rather than &$file. The same applies for a few other. Not sure if it matters.

•   $return = array(
      'validate' => NULL,

  - this default value breaks the contract of file_validate() - it should always return an array().

c960657, thanks for such a close review on a seemingly trivial patch. all are relevant points:

• now passing array's of named parameters rather than getting the full list of arguments
• i've removed the & byref because we're not making any changes.
• you're also correct that validate should return an array.

Just a few more nits:

+ * @param $op
+ *   One of the hook_file_* operations.

I don't think it is obvious that $op should be e.g. "load" instead of "file_load". I suggest changing the comment to One of the hook_file_* operations: "load", "validate", etc.

+ file_test_set_return('validate', array());
If validate returns an empty array by default, this line is redundant (though it may make the code easier to read - I am not sure).

-function file_test_file_load(&$file) {
-  $GLOBALS['file_test_results']['load'][] = func_get_args();
+function file_test_file_load($file) {

This particular hook is actually documented with &$file in the manual, but AFAICT this is a bug in the documentation, not a bug in the patch - so no problem here. $file is an object, so you can add properties that are visible to the caller even though it isn't passed by reference.

Apart from that it looks fine :-) It's great that you fixed the other bits that isn't directly related to the original issue.

If you do a reroll, you may also want to remove "a" from this line:
* Get the values passed to a the hook calls for a given operation.

c960657, thanks. corrected the @param phpdoc as you suggested, but decided to rework file_test_get_calls()'s description.

i also renamed _file_test_set_call() to _file_test_log_call() because it seemed more descriptive.

rtbc?

#276280: Tests needed: file.inc and #238299: file_create_url should return valid URLs are both blocked waiting on this.

I agree that _file_test_log_call() is a better function name.

+ * @param $op
+ *   One of the hook_file_* operations: "load", "validate", etc.

This looks good - but I think this comment (possible with outher strings listed) should be listed in all functions that take an $op parameter.

With that fix the patch looks RTBC to me.

added that comment to the other $op block. i don't think spelling them all out is worth the trouble because it'll just be another thing to change and if you're looking at the code it quite obvious which hooks are available.

Wow, this is much more betterer.

So basically, this is storing an array of all return values, along with their corresponding ops so that they can be checked rather than adjusting a global variable which happens to be in scope.

"+ *   One of the hook_file_[validate,download,references] operations."

This should match the other places $op is documented. There are at least two of these I saw.

+function _file_test_get_return($op) {
+function file_test_set_return($op, $value) {

Why is the getter private but the setter public? Let's pick one and be consistent. It seems to me there's no harm in public functions; this module is only enabled during the test run.

+  $results = variable_get('file_test_results', array());

Shouldn't these variable_get() calls call file_test_reset() instead if undefined, which initiates the values? Or is that too expensive because of the variable_set()? For example, it looks like:

+  $results = variable_get('file_test_results', array());
+  $results[$op][] = $args;

would generate a notice if file_test_results were undefined.

I will be eagerly watching this come back up to CNR/RTBC so I can get it committed tomorrow! :)

subscribing so I can test the re-roll if webchick doesn't beat me to it.

this is storing an array of all return values, along with their corresponding ops so that they can be checked rather than adjusting a global variable which happens to be in scope.

The just happens to be in scope is the key... when I was doing unittesting that didn't make HTTP requests to the test site it worked fine but once you try to start testing downloads it's no longer in scope and it stops working.

Why is the getter private but the setter public?

The public/private was done very deliberately to make it clear which are for the module's own use and which are for use by the callers. It was done to prevent any confusion on their roles.

"+ * One of the hook_file_[validate,download,references] operations."
This should match the other places $op is documented. There are at least two of these I saw.

They're actually totally different lists because only a small subset of the hooks (validate,download,references) return values. On the other hand they all take parameters so they're different lists.

We can't use file_test_reset() as a parameter to variable get for several reasons:
* file_test_reset() would be run every time the function was called--rather than just when variable_get() was unable to find the value and needed a default. Which would clear the results of each previous call to a hook.
* file_test_reset() works with two distinct variables, the hook parameters and return values. It's unclear which of these it should return as a value for the array.

As a compromise I've added a note to the module that the caller needs to initialize the module by calling file_test_reset().

since everyone seems to want the ops spelled out i'll relent...

whoops, i'd merged #330633: Temporary file cleanup needs some love (UnitTest included!) in with this...

All tests pass. My nitpicks are all lines that aren't affected by the patch. Would be nice to get this in so we can get t.d.o running over the weekend.

Thanks, fixed. :)