Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Waiting for this one before I'll continue with #1468328: Move file entity info, managed file, and file usage functionality into File module. Once this is done, it will be *much* easier to move all classes that depend on the file entity to file.module.

Niklas Fiekas’s picture

Assigned: Unassigned » Niklas Fiekas

Doing this now.

Niklas Fiekas’s picture

Assigned: Niklas Fiekas » Unassigned
Status: Active » Needs review
FileSize
224.81 KB

This patch:

  1. Moves helper functions from the .test file to the helper module, loading it where needed.
  2. Converts the file_test helper module to PSR-0. It has dummy stream wrappers for testing. Those are now in a lib folder in the testing module. I considered putting those into the system module's lib folder instead, but it seams to be better that way, because the module declares them and other testing modules have lib folders, too.
  3. Most importantly: Converts the test classes to PSR-0.
  4. Remove the file[] entry from system.info.

Status: Needs review » Needs work

The last submitted patch, 1598574-file-tests-psr-0-3.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
224.81 KB

While the test was postponed and waiting for an 8.x retest, other PSR-0 patches made it in. Yay! Chasing info file changes.

Status: Needs review » Needs work
Issue tags: -PSR-0

The last submitted patch, 1598574-file-tests-psr-0-5.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
Issue tags: +PSR-0

Number field tests failing looks pretty random to me. Going to test once more, before I am off to debugging this.

#5: 1598574-file-tests-psr-0-5.patch queued for re-testing.

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/lib/Drupal/system/Tests/File/TestBase.phpundefined
@@ -0,0 +1,213 @@
+class TestBase extends WebTestBase {

As discussed in IRC, this name is too short/generic. Same imho for HookTestBase.

I vote for FileTestBase.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
12.06 KB
224.92 KB

Yeah.

TestBase -> FileTestBase. HookTestBase -> FileHookTestBase.

Niklas Fiekas’s picture

Mhh ... how could I forget 6 classes? Well spotted, @aspilicious. This one should be complete.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Yes! Great work!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1598574-file-tests-psr-0-10.patch, failed testing.

aspilicious’s picture

$this->assertEqual($this->classname, file_stream_wrapper_get_class($this->scheme), t('Got correct class name for dummy scheme.'));

Is incorrect probably because the classname doesn't contain the namespace in it. Just hard code it for now.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
655 bytes
246.65 KB

This one should be fine.

aspilicious’s picture

Why can't we use single backslashes, I believe that is our standard.

Niklas Fiekas’s picture

Yeah ... no difference in single quotes strings unless at the very end, which is unlikely for class names. Only, that is the format the debug() statement I used to look up the class name serialized it.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Rdy!

RobLoach’s picture

#16: 1598574-file-tests-psr-0-16.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x! Thanks!

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