SimpleTest - WebTestBase method creates binary-text files when the intention was to create text files, and text file creation is broken

tgreen: interesting test failures. So that patch in #3 (which looks correct to me), is failing in:

FileFieldTestBase->getTestFile('text', 131072) in FileFieldTestBase.php 49

So it looks like without this patch, even though WebTestBase::drupalGetTestFiles() said it was generating "text" files of these sizes: array(16, 256, 1024, 2048, 20480) -- it was apparently generating one of size 131072. And with this patch that changes it from generating a sequence of "0"/"1" characters to generating random ASCII characters with codes in the range of 32 to 126, the file size is changing.

Ah... So the problem is that to break the file into lines, it calls wordwrap(). So if some character that wordwrap() recognizes as "a good place to break the line" occurs in those random characters, the file will have a variable number of breaks in it, whereas with all 0/1 characters, it just breaks every 64 characters.

So if you call the function with 'text', you get variable size files.

Hm...

It seems like our options are:
a) Leave the function behaving as it was (calling binary-text) -- I think on the other issue we at least fixed the documentation to say this was happening.
b) Use something other than wordwrap() to break the files into lines at exactly the passed-in number of characters.

I vote for (b), since the function is documented to have an exact number of chraracters per line.

So really what it should be doing I think is something like this:

for (i  goes from 1 to $lines) {
  for (j goes from 1 to $width) {
     $text .= (generate the character with the switch statement);
  }
  $text .= "\n";
}

and skip the call to wordwrap, which isn't really the right thing to do at all for 'text' files.

Thoughts?

I'm also going to suggest we fix the docs on the other issue because the file sizes are not as documented.

Let's see.

The errors in the test log are things like this:

Fatal error: Allowed memory size of 335544320 bytes exhausted (tried to allocate 274464721 bytes) in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/simpletest.module on line 517
FATAL Drupal\filter\Tests\FilterHtmlImageSecureTest: test runner returned a non-zero error code (255).

So it looks like your patch is making line 517 of simpletest.module run out of memory.

Ah... So I think you just need to get rid of the for(i = 0 to $size) loop. Looping over $lines and $width is enough. You are generating N-squared bytes instead of N bytes.

I do not think we need $size at all. See, the idea is that the outer loop on $lines will generate each line, and within that, the inner loop generates $width characters. That is what the function is actually documented to do. The previous behavior was to generate a bunch of characters ($size of them) in one big line and then use wordwrap() to insert \n characters.

So I guess that the inner loop should only generate $width - 1 characters for each line, to leave room for the \n character. That is what that size calculation previously did.

Right?

So if you leave in the $size loop, then you're generating each line and each character within each line $size times again, so you end up with N-squared or a very large number of characters, hence the memory problem.

The test bot seems to like it. :) The code looks right to me too.

My only minor gripe would be this comment:

+++ b/core/modules/simpletest/simpletest.module
@@ -502,26 +502,25 @@ function simpletest_classloader_register() {
+  // Generate random text and add \n after $width lines for symmetrical file.

I never thought that "symmetrical file" in the previous code that you got rid of made any sense, and I still don't.

What I think would be useful instead, if you want to add some actually useful comments to the code... keeping in mind that ideally, code comments tell "why?" not "how?":

- It seems to me that the doc block tells the reader what the function is supposed to do (or at least it will once we fix it on that other issue), so I don't think we need to repeat that in the in-code comments.
- One thing we might want to comment on is the $width - 1 piece, something like "Generate $width - 1 characters to leave space for the "\n" character".

Honestly, I think the code is pretty clear with just that one comment in it, because if I see (for i = 1 to $lines) that immediately says "I'm looping over all the lines", and (for j = 1 to $width) says "I'm looping over the width of each line". So we don't need a comment saying "We're looping over lines and over the characters in each one" because the local variables have good names and the code is pretty simple.

Right?

A binary file doesn't really have the concept of "lines", so it's a bit weird to see the newlines in there, but as this was already the case before it's fine I guess.

The solution looks good, but this still needs a test.

prfenssen: In this case, the file has always had "lines" for binary files, yes. Agreed it is a bit odd, but it would probably break even more to change that at this point.

And regarding a test, this could be useful, but the existing test coverage actually found the problem with using wordwrap() for generating 'text' files. We have a lot of tests that are using these generated files... I am not entirely convinced we need another one added? It seems like the coverage is adequate. For the moment, removing the "needs tests" tag.

Looks great!

+++ b/core/modules/simpletest/simpletest.module
@@ -502,26 +502,25 @@ function simpletest_classloader_register() {
+      // Generate $width - 1 characters to leave space for the "\n" character

Nitpick: this line of comment needs to end in a .

There we go, thanks!

Updating summary and adding beta eval.

Note that if #2299361: description of drupalGetTestFiles() is vague and has a typo; simpletest_generate_file() needs help too goes in first, we should return here and fix the docs in this patch in accordance with the patch here (which changes the type of files to text instead of binary-text).

So glad we figured out the file size question. Happy to be working on core too.

Adding liberatr to commit message (helped trgreen with programming in person even though did not add a patch directly).

I think that we could open a followup to add explicit test coverage - since it would be useful.

Committed 2219b31 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

.

Fixed the exceptions.

.

Thanks! This looks like the same patch as 8.x, and I think we should commit it to 7.x, as it is a bug.

That patch messes up the code indentation, which also might make it a bit harder to review, so here's a version that fixes that.

I'm leaving this RTBC since I didn't change anything except the indentation.

Actually no reason to mess with blank lines either...

Added an issue to fix the indentation in Drupal 8 also.

Committed to 7.x - thanks!