See #2862508: Convert system functional tests to phpunit
See #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)

Scope:
Everything in /core/modules/system/src/Tests/Module

CommentFileSizeAuthor
#17 2930072-12.patch4.68 KBLendude
#14 x100_InstallUninstallTest-phpunit.patch1.23 KBAnonymous (not verified)
#14 x100_InstallUninstallTest-simpletest.patch524 bytesAnonymous (not verified)
#12 interdiff-6-12.txt865 bytesAnonymous (not verified)
#12 2930072-12.patch4.68 KBAnonymous (not verified)
#6 interdiff-4-6.txt677 bytesAnonymous (not verified)
#6 2930072-6.patch4.79 KBAnonymous (not verified)
#4 2930072-4.patch4.73 KBAnonymous (not verified)
#2 2930072-2.patch3.86 KBAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

vaplas created an issue. See original summary.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Active » Needs review
FileSize
2930072-2.patch3.86 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2930072-2.patch, failed testing. View results

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review
FileSize
2930072-4.patch4.73 KB

Status: Needs review » Needs work

The last submitted patch, 4: 2930072-4.patch, failed testing. View results

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review
FileSize
2930072-6.patch4.79 KB
interdiff-4-6.txt677 bytes
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Thank you @vaplas for trying to tackle this beast :) So far the changes look minimal, but it all depends whether the tests are actually passing or not.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

Thanks for the review, @dawehner!

When I started the conversion, I forgot to delete a couple of unsuccessful experiment modules from the contrib 'modules' folder (without config schemes and help hooks). And InstallUninstallTest works with fails. Therefore, something is definitely checkes :)

Although it seems strange to me that InstallUninstallTest checkes all the modules, not only in core folder.

Status: Needs review » Needs work

The last submitted patch, 6: 2930072-6.patch, failed testing. View results

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review

Random failure with apc memory.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented

@vaplas++

+++ b/core/modules/system/tests/src/Functional/Module/VersionTest.php
@@ -48,8 +48,8 @@ public function testModuleVersions() {
+      $checkbox = $this->xpath('//input[@id="edit-modules-module-test-enable" and @disabled="disabled"]');
+      $this->assertEqual(!empty($checkbox), $i % 2, $dependencies[$i]);

Couldn't this just be $this->assertEqual(!empty($checkbox[0]->getAttribute('disabled')), $i % 2, $dependencies[$i]);? Then there is no need to change the xpath.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
2930072-12.patch4.68 KB
interdiff-6-12.txt865 bytes

@Lendude, thanks for nice point! This helps not only to halve the change in the code (:p, I love such statistics), but perhaps even more compatible. Because it keeps checking the attribute value, not the number of items (as in my previous version). Done.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented 
+++ b/core/modules/system/tests/src/Functional/Module/HookRequirementsTest.php
diff --git a/core/modules/system/src/Tests/Module/InstallUninstallTest.php b/core/modules/system/tests/src/Functional/Module/InstallUninstallTest.php
similarity index 99%

similarity index 99%
rename from core/modules/system/src/Tests/Module/InstallUninstallTest.php

rename from core/modules/system/src/Tests/Module/InstallUninstallTest.php
rename to core/modules/system/tests/src/Functional/Module/InstallUninstallTest.php

Wow I cannot believe that this just works :) I fear we should have a couple of more test runs to prove this doesn't fail more often than right now :( Any idea how we should tackle this ideally?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
x100_InstallUninstallTest-simpletest.patch524 bytes
x100_InstallUninstallTest-phpunit.patch1.23 KB

Yes, I have already tried to make more test runs

In both case 'Build timed out' after 124 runs. I suspect that with simpletest when the test runs, there is a leak of resources that lead to the crash of CI:

https://dispatcher.drupalci.org/job/drupal_patches/40289/console

20:20:28 Drupal\system\Tests\Module\InstallUninstallTest              3207 passes                                      
21:28:21 Build timed out (after 110 minutes). Marking the build as aborted.

1 hour between last test and time out. But maybe the problem is in the environment. So, let's check by 100 runs.

The last submitted patch, 14: x100_InstallUninstallTest-simpletest.patch, failed testing. View results

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

Phpunit version works more slower, but stability (at least more stable than simpletest version).

Okay. Then why did #6 fail first? Because not only InstallUninstallTest (simpletest) is a generator of such fails. #2926309: Random fail due to APCu not being able to allocate memory

So, if we convert this into a phpunit version, we can reduce the frequency of these fails.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented
Status: Needs review » Reviewed & tested by the community
FileSize
2930072-12.patch4.68 KB

@vaplas so looks like #6 was just a fluke right?

So then this looks all good and nice minimal changes!

Just re-upping the patch from #12 to make clear which patch is RTBC.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

Yes, it is similar to the fact that InstallUninstallTest-simpletest, like and rest tests, exhausts some resources of CI very quickly. Perhaps InstallUninstallTest-phpunit also eating these resources, but to a lesser extent. Therefore, it works more steadily (#14).

Unfortunately, we have not yet figured out exactly the root of all evils. But it seems, that #17 is definitely not a step back :)

catch’s picture

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

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

  • catch committed 3cf0815 on 8.5.x 
    Issue #2930072 by vaplas, Lendude: Module: Convert system functional...

  • catch committed e0f9cdc on 8.4.x 
    Issue #2930072 by vaplas, Lendude: Module: Convert system functional...

Status: Fixed » Closed (fixed)

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