Comments

michielnugter created an issue. See original summary.

Antonnavi’s picture

Status: Active » Needs review
FileSize
100.97 KB

Status: Needs review » Needs work

The last submitted patch, 2: browsertest-update-2870464-1.patch, failed testing.

dawehner’s picture

Hi @Antonnavi, thank you for your contribution!

First tip: Try to use the following git configuration:

[diff]
  renames = copies

This detects renames, see https://www.drupal.org/documentation/git/configure for more details. This will make the patch size significantly smaller.

Here is some quick analysis of the failures:

1) Drupal\Tests\update\Functional\FileTransferAuthorizeFormTest::testViaAuthorize
copy(/var/www/html/core/modules/update/tests/src/Functional/../../tests/update_test_new_module/8.x-1.0/update_test_new_module.tar.gz): failed to open stream: No such file or directory

/var/www/html/core/modules/update/tests/src/Functional/FileTransferAuthorizeFormTest.php:27

FAILURES!
Tests: 1, Assertions: 5, Errors: 1.

You need to adapt the filepaths in places like core/modules/update/src/Tests/UpdateUploadTest.php:66

Update.Drupal\Tests\update\Functional\UpdateCoreTest

These tests rely on batch api to work, see #2855942: Create ::checkForMetaRefresh() on BrowserTestBase. Maybe try to get that issue over the finish line first :)

michielnugter’s picture

Component: phpunit » update.module
Issue tags: +phpunit initiative
naveenvalecha’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
5.42 KB

Here's the fresh patch with expected failures.
Postponed on #2870009: Update: Convert system functional tests to phpunit

//Naveen

Status: Needs review » Needs work

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

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
7.84 KB
3.37 KB

Here we go with fixes

Self Review of Interdiff:

  1. +++ b/core/modules/update/tests/src/Functional/UpdateTestBase.php
    @@ -58,7 +58,7 @@ protected function setUp() {
    +   * @see \Drupal\update_test\Controller\UpdateTestController::updateTest()
    

    This @see link was broken so I fixed it in this conversion. I hope that should be fine. If anyone will object for it, we could remove it from this patch and create a follow-up issue.

  2. +++ b/core/modules/update/tests/src/Functional/UpdateTestBase.php
    @@ -69,6 +69,7 @@ protected function refreshUpdateStatus($xml_map, $url = 'update-test') {
    +    $this->drupalGet('admin/reports/updates');
    

    We need to send the get request again because WTB::clickLink which calls WTB::clickLinkHelper sends a new drupalget request to click the link that refresh the page but BTB::clickLink fires on the NodeElement and it does refresh the whole page. So refreshing it again so that we'll get the updated page. See the code of WTB::clickLinkHelper for reference.

      protected function clickLinkHelper($label, $index, $pattern) {
        // Cast MarkupInterface objects to string.
        $label = (string) $label;
        $url_before = $this->getUrl();
        $urls = $this->xpath($pattern, [':label' => $label]);
        if (isset($urls[$index])) {
          $url_target = $this->getAbsoluteUrl($urls[$index]['href']);
          $this->pass(SafeMarkup::format('Clicked link %label (@url_target) from @url_before', ['%label' => $label, '@url_target' => $url_target, '@url_before' => $url_before]), 'Browser');
          return $this->drupalGet($url_target);
        }
        $this->fail(SafeMarkup::format('Link %label does not exist on @url_before', ['%label' => $label, '@url_before' => $url_before]), 'Browser');
        return FALSE;
      }

//Naveen

Status: Needs review » Needs work

The last submitted patch, 8: 2870464-8.patch, failed testing. View results

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
9.33 KB
1.87 KB

Here we go with the more fixes.

Self Review of the interdiff attached:

  1. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
    @@ -76,6 +79,7 @@ public function testNormalUpdateAvailable() {
    +        $this->drupalGet('admin/reports/updates');
    

    This is required to refresh the page. See #8.2 for more details.

  2. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
    @@ -139,6 +143,7 @@ public function testMajorUpdateAvailable() {
    +          $this->drupalGet('admin/reports/updates');
    

    Ditto as above.

Expected Failure:

Drupal\Tests\update\Functional\UpdateContribTest::testUpdateBrokenFetchURL
'Checked available update data for 3 projects.' found only once on the page
Failed asserting that 0 is identical to 1.

/var/www/html/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php:168
/var/www/html/core/modules/update/tests/src/Functional/UpdateContribTest.php:372

FAILURES!
Tests: 8, Assertions: 72, Failures: 1.

This test is still failing as it is trying to assert the Drupal set messages which are not present because we're refreshing the page explicitly as per #8.2. What're the thoughts on #8.2?

Status: Needs review » Needs work

The last submitted patch, 10: 2870464-10.patch, failed testing. View results

Lendude’s picture

@naveenvalecha Looks like the solution to #8.2 is not using another drupalGet() but using checkForMetaRefresh() since the click triggers a batch operation.

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
10.92 KB
4.1 KB

Thanks Pal!

//naveen

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I like that this api is kinda explicit

  • Gábor Hojtsy committed fa54442 on 8.4.x
    Issue #2870464 by naveenvalecha, Antonnavi, dawehner, Lendude: Convert...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, the changes look good. The depreciations and trigger_error() sections match the existing patterns used for tests. Committed!

Gábor Hojtsy’s picture

(Note I did not merge to 8.3.x because the depreciation should only start in 8.4.0).

naveenvalecha’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Fixed » Needs work
Issue tags: +Needs reroll

Test conversion should go in both branches, similarly with the previous conversion issues. This patch could be rerolled for 8.3.x without deprecation notices.

//Naveen

Jo Fitzgerald’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
12.29 KB
997 bytes

Re-roll for 8.3.x without deprecation notices, as suggested by @naveenvalecha. Although I suspect that @Gábor Hojtsy meant that he does not intend to commit this to 8.3.x.

Gábor Hojtsy’s picture

I'm honestly not 100% clear on the policy here, will ask other committers :)

  • Gábor Hojtsy committed 25168a0 on 8.3.x
    Issue #2870464 by naveenvalecha, Antonnavi, dawehner, Lendude: Convert...
Gábor Hojtsy’s picture

Status: Needs review » Fixed

Ok discussed with @catch. He said so long as the class is not part of the testing infrastructure/API itself, just as in this case a base class to simplify writing tests for the module itself, we can backport the patch as-is. It is unlikely that contributed modules extended the class.

So merged the original commit. Sorry for making you work on this, should have asked other committers first.

naveenvalecha’s picture

Thanks, Gabor! for committing and Jo Fitzgerald! for the patch.

Jo Fitzgerald’s picture

Thanks, Gabor. Don't worry about the extra work - a 997 byte interdiff doesn't take long to produce!

Status: Fixed » Closed (fixed)

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