The patch at https://drupal.org/node/2140433 in #39 should have had 2 fails, but only had 1.

Locally run-tests.sh failed to actually run the test method in MTimeProtectedFileStorageTest, but runnint it with phpunit I saw an additional failure.

Note: This is actually a phpunit "issue":

that it won't discover a test that's a subclass of another concrete test

CommentFileSizeAuthor
#14 interdiff-2202611-14.txt1.15 KBdamiankloip
#14 2202611-14.patch10.09 KBdamiankloip
#8 2202611-8.patch11.41 KBpwolanin
#6 2202611-6.patch11 KBdamiankloip
#4 interdiff-2202611-4.txt10.11 KBdamiankloip
#4 2202611-4.patch8.9 KBdamiankloip
#2 2202611.patch10.08 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Title: PhpStorage\MTimeProtectedFastFileStorageTest runs with run-tests.sh, but MTimeProtectedFileStorageTest does not » MTimeProtectedFastFileStorageTest runs with run-tests.sh, but MTimeProtectedFileStorageTest does not
damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Active » Needs review
FileSize
2202611.patch10.08 KB

This is a problem with test discovery and PHPUnit. I am guess you could run the test fine in isolation, but running the whole suite would probably not run this test. Simpletest just uses PUPUnit's discovery mechanism to find the test classes anyway...

So, I think we just move what's in MTimeProtectedFileStorageTest into the abstract PhpStorageTestBase class, and make both MTimeProtectedFastFileStorageTest and MTimeProtectedFileStorageTest extend that instead of MTimeProtectedFastFileStorageTest extending MTimeProtectedFileStorageTest.

This should now get discovered and run just fine.

Status: Needs review » Needs work

The last submitted patch, 2: 2202611.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
2202611-4.patch8.9 KB
interdiff-2202611-4.txt10.11 KB

Oops, sorry. We need to introduce another base class for the MTimeProtected* classes instead. This can't all live in PhpStorageTestBase as the simple file storage test uses this too.

Let's also introduce a MTimeProtectedFileStorageBase class.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

It would be clearer if you posted the patch using git diff --no-renames

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
2202611-6.patch11 KB

Really?! Not really worth the effort IMO (This is a pretty simple patch) but OK. There you go.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

6: 2202611-6.patch queued for re-testing.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
2202611-8.patch11.41 KB

re-roll for conflict from #2140433: Port SA-CORE-2013-003 to Drupal 8, plus doxygen and other minor cleanup.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Where's the interdiff?

Also, you ask for the patch with no renames. Then don't review it anyway...

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@damiankloip - I did review the #6 patch when I re-tested it, but then saw the conflict before I posted my comment.

The interdiff would just be fixing the @file doxygen, and removing the duplicated line:

$this->storageFactory = new PhpStorageFactory;

that was in the new base class setup() method.

In terms of review.

I tested after re-rolling. scripts/run-tests.sh now finds both tests as expected.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Ok, thanks for confirming. So now we just get someone else to rtbc and we're good to go.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageBase.php
@@ -0,0 +1,113 @@
+  function setUp() {
...
+  function testCRUD() {
...
+  function testSecurity() {

It would be great to use the proper visibility here.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue summary: View changes
damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
2202611-14.patch10.09 KB
interdiff-2202611-14.txt1.15 KB

Right on!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Thank you!

Can someone please open an issue on the testbot + drupal to not execute phpunit tests via the UI/run-tests.sh anymore?
Yeah just wanted to force someone to work on that.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

That's already open #2189317: Get a PHPUnit job type running that is separate and independent from the Simpletest Job type, fix the d.o integration so that we can run the PHPUnit tests independently.

Committed/pushed this one to 8.x, thanks!

Status: Fixed » Closed (fixed)

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