Add test trait for drupalGetTestFiles and drupalCompareFile

This seems to work for me.

I feels a bit odd putting it in the simpletest module, but it does follow the same principle of Drupal\simpletest\ContentTypeCreationTrait, which I'm also using for my BrowserTestBase tests.

This fixes a dumb spacing error at the end of the file.

As far as testing, I'm not completely sure. Maybe something like ImageFieldDefaultImagesTest::testDefaultImages()?

The attached patch converts simpletest_generate_file() to a method.

Ah, good idea, and that seems to work (and I'm assuming you meant WebTestBase).

When I add use TestFileCreationTrait; to the WebTestBase class, the tests pass. Without it they fail. The interdiff is for the(hopefully) passing patch.

1. +++ b/core/modules/simpletest/src/TestFileCreationTrait.php
   @@ -0,0 +1,154 @@
   +   * Any subsequent calls will not generate any new files, or copy the files
   +   * over again. However, if a test class adds a new file to public:// that
   +   * is prefixed with one of the above types, it will get returned as well, even
   +   * on subsequent calls.
   

   This is such weird behaviour. Not the fault of this patch but it is just strange.

2. +++ b/core/modules/simpletest/src/TestFileCreationTrait.php
   @@ -0,0 +1,154 @@
   +    if (empty($this->generatedTestFiles)) {
   

   This property should probably be part of the trait.

3. +++ b/core/modules/simpletest/src/TestFileCreationTrait.php
   @@ -0,0 +1,154 @@
   +      $original = drupal_get_path('module', 'simpletest') . '/files';
   +      $files = file_scan_directory($original, '/(html|image|javascript|php|sql)-.*/');
   ...
   +        file_unmanaged_copy($file->uri, PublicStream::basePath());
   ...
   +      $files = file_scan_directory('public://', '/' . $type . '\-.*/');
   

   The dependencies on these Drupal common functions is interesting.

4. +++ b/core/modules/simpletest/src/TestFileCreationTrait.php
   @@ -0,0 +1,154 @@
   +  public static function generateFile($filename, $width, $lines, $type = 'binary-text') {
   

   Why public static? I guess it could be useful. Just to call this without using the trait. Dunno.

Added property.

generateFile is public static because it's a replacement for a global simpletest function: simpletest_generate_file(). Not sure if it really makes sense to do it this way since it will still need a class to use the trait in order to call it. If not, what would be the recommended approach? It would probably be good for the function to be available after simpletest goes away.

Relating an issue from the paragraphs module that is postponed on this.

I think this counts as a backwards compatibility issue.

I added the trait (ImageFieldCreationTrait) from #2782309: Refactor File and Image related image field creation logic into a new trait that wasn't included in this issue.

From the test log:
17:47:52 PHP Fatal error: Trait 'Drupal\image\Tests\ImageFieldCreationTrait' not found in /var/www/html/core/modules/image/src/Tests/ImageFieldTestBase.php on line 25
Not sure why that would happen, do I need to import this trait even though it's in the same namespace?

EDIT: The review is against #21

1. +++ b/core/tests/Drupal/Tests/TestFileCreationTrait.php
   @@ -0,0 +1,169 @@
   +  protected $generatedTestFiles = FALSE;
   

   This is a flag but the naming doesn't sound as for a flag/bool variable. $testFilesGenerated, $testFilesAreGenerated, $hasGeneratedTestFiles? I have no idea but with $generatedTestFiles I'm expecting to find there a list/array.

2. +++ b/core/tests/Drupal/Tests/TestFileCreationTrait.php
   @@ -0,0 +1,169 @@
   +      // Copy other test files from simpletest.
   +      $original = drupal_get_path('module', 'simpletest') . '/files';
   

   Well, I think we should break the dependency on Simpletest. This is because I guess we'll use this trait also an BTB & Co. and that should not rely on simpletest. So, we need a home for the sample files.

3. +++ b/core/tests/Drupal/Tests/TestFileCreationTrait.php
   @@ -0,0 +1,169 @@
   +    // Make sure type is valid.
   +    if (in_array($type, ['binary', 'html', 'image', 'javascript', 'php', 'sql', 'text'])) {
   

   What if type is invalid? Probably throwing an error would be better.

4. +++ b/core/tests/Drupal/Tests/TestFileCreationTrait.php
   @@ -0,0 +1,169 @@
   +   * Compare two files based on size and file name.
   

   s/Compare/Compares ?

5. +++ b/core/tests/Drupal/Tests/TestFileCreationTrait.php
   @@ -0,0 +1,169 @@
   +   * @param \stdclass $file1
   

   s/stdclass/stdClass

6. +++ b/core/tests/Drupal/Tests/TestFileCreationTrait.php
   @@ -0,0 +1,169 @@
   +  protected function compareFiles($file1, $file2) {
   

   Should we add that this is used in uasort() in \Drupal\Tests\TestFileCreationTrait::getTestFiles() and also add a @see?

7. +++ b/core/tests/Drupal/Tests/TestFileCreationTrait.php
   @@ -0,0 +1,169 @@
   +  public static function generateFile($filename, $width, $lines, $type = 'binary-text') {
   

   I don't see any reason in core why this would be public. But, who knows, maybe somebody has used it in contrib or custom?

I did items 1, 4, 5, 6.

2 - Where should they go?

3 - Maybe make separate issues for changes in functionality? This ticket is about making the existing functions available for the new testing system.

7 - Instructions unclear.

+++ b/core/tests/Drupal/Tests/TestFileCreationTrait.php
@@ -0,0 +1,171 @@
+      // Copy other test files from simpletest.
+      $original = drupal_get_path('module', 'simpletest') . '/files';

Even it's just about moving the code around, the source files were not moved. We still keep the dependency to Simpletest. I propose to move them in core so, even Simpletest can benefit. Where? I don't know. How it sounds core/tests/files?

What about using core/tests/fixtures?

Yes, why not? Let's do that.

Thank you @jmuzz, looks good.

Just a minor:

+++ b/core/tests/Drupal/Tests/TestFileCreationTrait.php
@@ -67,8 +67,8 @@ protected function getTestFiles($type, $size = NULL) {
+      $original = \Drupal::root() . '/core/tests/fixtures/files';

I think \Drupal::root() is not needed. Simply 'core/tests/fixtures/files' should work.

Also let's see if other tests were not broken. I'm thinking on tests that are accessing directly the files in simpletest dir (I remember I saw once such test).

I see what you mean. Well, as I understand it the point of this is to make backwards compatibility so the existing tests can get ported without doing a lot of extra stuff to them so that extra stuff can be done later. Messing with a bunch of the tests to make them compatible with the backwards compatibility seems to defeat the purpose. Maybe the files should be left where they are for now.

It turns out in #45 that decoupling Simpletest is not so easy. Also, we still have to keep files in simpletest for the case some contrib tests rely on those files. Probably it's better not to break here the dependency on simpletest but to deal with that in a future issue, as @jmuzz pointed in #44. Although, conceptually we have a clear issue here: a core functionality (BTB tests) will have a dependency on a module (Simpletest).

I'm reposting #30 and set back to RTBC as per #35 to unblock conversions. I opened #2803621: Break BrowserTestBase & children dependency on Simpletest, deprecate stub BC traits for breaking this dependency, probably after all test conversions.

Since this is just a code shuffle in the test system to make something available to BrowserTestBase I'm going to commit to 8.2.x as well.

Committed and pushed 97b2c76 to 8.3.x and e4bac87 to 8.2.x. Thanks!

Formal deprecation here: #3028708: Deprecate simpletest_generate_file()