Problem/Motivation

WebTestBase used to have a way to generate test files.

Proposed resolution

Let's provide a trait for that.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#47 2738567-30.patch10.32 KBclaudiu.cristea
#45 2738567-45.patch29.73 KBclaudiu.cristea
#45 interdiff.txt15.55 KBclaudiu.cristea
#41 interdiff-30-41.txt5.84 KBjmuzz
#41 2738567-41.patch14.86 KBjmuzz
#31 interdiff-28-30.txt2.18 KBjmuzz
#30 2738567-30.patch10.32 KBjmuzz
#30 2738567-30.patch10.32 KBjmuzz
#28 interdiff-21-28.txt2.59 KBjmuzz
#28 2738567-28.patch11.5 KBjmuzz
#22 interdiff-2738567-21-22.txt4.88 KBsamuel.mortenson
#22 2738567-22.patch15.38 KBsamuel.mortenson
#21 interdiff.txt3.73 KBdawehner
#21 2738567-21.patch10.24 KBdawehner
#17 interdiff-15-17.txt1.13 KBjmuzz
#17 file-test-2738567-17.patch9.57 KBjmuzz
#15 interdiff-12-15.txt450 bytesjmuzz
#15 file-test-2738567-15.patch9.26 KBjmuzz
#12 interdiff-7-12.txt577 bytesjmuzz
#12 file-test-2738567-12.patch9.26 KBjmuzz
#7 interdiff-2738567-7.txt3.86 KBmtift
#7 file-test-2738567-7-FAIL.patch8.91 KBmtift
#7 file-test-2738567-7.patch9.09 KBmtift
#5 interdiff-2738567-5.txt3.05 KBmtift
#5 file-test-2738567-5.patch5.24 KBmtift
#3 file-test-2738567-3.patch3.73 KBmtift
#2 2738567-3-file-test.patch3.73 KBmtift
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

dawehner created an issue. See original summary.

mtift’s picture

Status: Active » Needs review
FileSize
3.73 KB

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.

mtift’s picture

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

dawehner’s picture

Cool, thank you! Can you think of a core test we could convert to BTB at the same time to prove that this trait actually works?

+++ b/core/modules/simpletest/src/TestFileCreationTrait.php
@@ -0,0 +1,106 @@
+        simpletest_generate_file('binary-' . $count++, 64, $line, 'binary');

In order to be helpful for browser tests I think you need to put this function onto this trait. You could make it a public static function, which adds a working BC layer.

mtift’s picture

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

The attached patch converts simpletest_generate_file() to a method.

dawehner’s picture

One thing to prove that this trait actually works could be to replace the existing code in TestBase by this trait. Do you think this would make sense?

mtift’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 7: file-test-2738567-7-FAIL.patch, failed testing.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

Great to see it works.

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

Ah yeah, I wasn't there whether its in TestBase or in WebTestBase.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jmuzz’s picture

Status: Needs work » Needs review
FileSize
9.26 KB
577 bytes

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.

jmuzz’s picture

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

Status: Needs review » Needs work

The last submitted patch, 12: file-test-2738567-12.patch, failed testing.

jmuzz’s picture

Status: Needs work » Needs review
FileSize
9.26 KB
450 bytes

Status: Needs review » Needs work

The last submitted patch, 15: file-test-2738567-15.patch, failed testing.

jmuzz’s picture

Status: Needs work » Needs review
FileSize
9.57 KB
1.13 KB
jmuzz’s picture

I think this counts as a backwards compatibility issue.

dawehner’s picture

index 0000000..8be0737
--- /dev/null

--- /dev/null
+++ b/core/modules/simpletest/src/TestFileCreationTrait.php

+++ b/core/modules/simpletest/src/TestFileCreationTrait.php
@@ -0,0 +1,161 @@

Could we move this trait outside of the simpletest namespace? This is no longer about simpletest to be honest.

dawehner’s picture

Status: Needs review » Needs work
dawehner’s picture

Status: Needs work » Needs review
FileSize
10.24 KB
3.73 KB

Just moved stuff around / rename methods as well as some minor docs.

samuel.mortenson’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 22: 2738567-22.patch, failed testing.

samuel.mortenson’s picture

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?

dawehner’s picture

@samuel.mortenson
Well, strictly speaking this doesn't belong into this issue. This is about drupalGetTestFiles and drupalGetTestFiles
and the other one is about the file.module/image.module specific test base classes.

dawehner’s picture

@samuel.mortenson
Do you mind reviewing #21 as it is?

claudiu.cristea’s picture

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?

jmuzz’s picture

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.

dawehner’s picture

The problem with the renaming is that existing tests using this trait will have a BC break :(
This issue was meant to just move code around :(

jmuzz’s picture

jmuzz’s picture

FileSize
2.18 KB

Status: Needs review » Needs work

The last submitted patch, 30: 2738567-30.patch, failed testing.

The last submitted patch, 30: 2738567-30.patch, failed testing.

jmuzz’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This was just about moving the code. Of course we could improve things, but those should be done in other issues

mtift’s picture

claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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?

dawehner’s picture

What about using core/tests/fixtures?

claudiu.cristea’s picture

Status: Needs review » Needs work

What about using core/tests/fixtures?

Yes, why not? Let's do that.

dawehner’s picture

I agree with @claudiu.cristea, let's do it right!

jmuzz’s picture

Status: Needs work » Needs review
FileSize
14.86 KB
5.84 KB
claudiu.cristea’s picture

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).

Status: Needs review » Needs work

The last submitted patch, 41: 2738567-41.patch, failed testing.

jmuzz’s picture

Status: Needs work » Needs review

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.

claudiu.cristea’s picture

FileSize
15.55 KB
29.73 KB

Status: Needs review » Needs work

The last submitted patch, 45: 2738567-45.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
10.32 KB

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.

claudiu.cristea’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +rc eligible

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!

  • alexpott committed 97b2c76 on 8.3.x
    Issue #2738567 by jmuzz, mtift, claudiu.cristea, dawehner, samuel....

  • alexpott committed e4bac87 on 8.2.x
    Issue #2738567 by jmuzz, mtift, claudiu.cristea, dawehner, samuel....

Status: Fixed » Closed (fixed)

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