As per the parent issue.


./Update/AutomatedCronUpdateWithAutomatedCronTest.php
./Update/AutomatedCronUpdateWithoutAutomatedCronTest.php
./Update/ConfigOverridesUpdateTest.php
./Update/DbUpdatesTrait.php
./Update/DependencyHookInvocationTest.php
./Update/DependencyMissingTest.php
./Update/DependencyOrderingTest.php
./Update/FieldSchemaDataUninstallUpdateTest.php
./Update/FilterHtmlUpdateTest.php
./Update/InstallProfileSystemInstall8300Test.php
./Update/InvalidUpdateHookTest.php
./Update/LocalActionsAndTasksConvertedIntoBlocksUpdateTest.php
./Update/MenuTreeSerializationTitleFilledTest.php
./Update/MenuTreeSerializationTitleTest.php
./Update/PageTitleConvertedIntoBlockUpdateTest.php
./Update/RecalculatedDependencyTest.php
./Update/RouterIndexOptimizationFilledTest.php
./Update/RouterIndexOptimizationTest.php
./Update/SevenSecondaryLocalTasksConvertedIntoBlockUpdateTest.php
./Update/SiteBrandingConvertedIntoBlockUpdateTest.php
./Update/StableBaseThemeUpdateTest.php
./Update/UpdateEntityDisplayTest.php
./Update/UpdatePathRC1TestBaseFilledTest.php
./Update/UpdatePathRC1TestBaseTest.php
./Update/UpdatePathTestBase.php
./Update/UpdatePathTestBaseFilledTest.php
./Update/UpdatePathTestBaseTest.php
./Update/UpdatePathTestJavaScriptTest.php
./Update/UpdatePathWithBrokenRoutingFilledTest.php
./Update/UpdatePathWithBrokenRoutingTest.php
./Update/UpdatePostUpdateFailingTest.php
./Update/UpdatePostUpdateTest.php
./Update/UpdateSchemaTest.php
./Update/UpdateScriptTest.php
./Update/UpdatesWith7xTest.php

Comments

dawehner created an issue. See original summary.

dawehner’s picture

Status: Active » Needs review
FileSize
39.11 KB

I tried starting with this ... but yeah it doens't work yet, I believe the batch process doesn't work yet.

Status: Needs review » Needs work

The last submitted patch, 2: 2870009-2.patch, failed testing.

naveenvalecha’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
39.26 KB

Just a reroll for now.

Status: Needs review » Needs work

The last submitted patch, 5: 2870009-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Saviktor’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

I could apply the patch, so no reroll is needed

kiamlaluno’s picture

Status: Needs review » Needs work

The patch could apply, but it needs to be changed basing on the automated coding standards fixes. Also, the patch failed testing.

Lendude’s picture

Status: Needs work » Needs review
FileSize
4.23 KB
39.86 KB

@dawehner really amazing how far you got with this.

Batch is in BrowserTestBase now, so lets see if we can move this. Cleared up some fails, cleaned up some debug code. \Drupal\FunctionalTests\Update\UpdatePathTestBaseTest passes locally now, so lets see what is still broken here.

Status: Needs review » Needs work

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

Lendude’s picture

Status: Needs work » Needs review
FileSize
4.05 KB
39.67 KB

Bleh that was rolled against 8.3.x, this is it for 8.4.x

Status: Needs review » Needs work

The last submitted patch, 11: 2870009-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Lendude’s picture

These tests pass locally on both SQlite and MySQL :(

dawehner’s picture

\Drupal\FunctionalTests\Update\UpdatePathTestBaseTest passes locally now, so lets see what is still broken here.

Wow, this is amazing! This is at least one of the really painful conversions we kinda have in front of us.

kiamlaluno’s picture

See the changes suggested from PHP_CodeSniffer in #12.

--- tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
+++ PHP_CodeSniffer
@@ -14,7 +14,6 @@
 use Drupal\Core\DependencyInjection\ContainerBuilder;
 use Drupal\Core\Language\Language;
 use Drupal\Core\Url;
-use Drupal\simpletest\WebTestBase;
 use Drupal\user\Entity\User;
 use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\HttpFoundation\Request;
--- modules/simpletest/src/WebTestBase.php
+++ PHP_CodeSniffer
@@ -8,7 +8,6 @@
 use Drupal\Component\Utility\NestedArray;
 use Drupal\Component\Utility\UrlHelper;
 use Drupal\Component\Utility\SafeMarkup;
-use Drupal\Core\Database\Database;
 use Drupal\Core\EventSubscriber\AjaxResponseSubscriber;
 use Drupal\Core\EventSubscriber\MainContentViewSubscriber;
 use Drupal\Core\Session\AccountInterface;

I didn't check the code, but PHP_CodeSniffer is suggesting that two use lines are referring a class that is then not used.

dawehner’s picture

Status: Needs work » Needs review
FileSize
40.97 KB
3.95 KB

I did a bit of investigation.

Status: Needs review » Needs work

The last submitted patch, 16: 2870009-16.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dawehner’s picture

Status: Needs work » Needs review
FileSize
41.05 KB
646 bytes

This is commenting out $originalContainer on the FunctionSetupTrait. Does someone has a better suggestion?

Status: Needs review » Needs work

The last submitted patch, 18: 2870009-18.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Lendude’s picture

Status: Needs work » Needs review
FileSize
1.51 KB
40.98 KB

Per the PHP manual:
Note:
Prior to PHP 7.0, defining a property in a class with the same name as in a trait would throw an E_STRICT if the class definition was compatible (same visibility and initial value).

So the best solution would be to move to PHP 7 :-)

Lets see what this does.

dawehner’s picture

That is a good solution for now! Thank you @lendude

In the meantime we could leverage @propertyhttps://phpdoc.org/docs/latest/references/phpdoc/tags/property.html if we care about it :)

Status: Needs review » Needs work

The last submitted patch, 20: 2870009-20.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Lendude’s picture

Status: Needs work » Needs review
FileSize
1.15 KB
41.04 KB

require vs require_once ....

This now comes back green locally with APC switched on.

dawehner’s picture

I don't understand that, can you elaborate?

Lendude’s picture

Assigned: Unassigned » Lendude

I honestly have no idea why require_once doesn't work here with APC on, but it just returns TRUE, which makes me believe that somehow that file was loaded before (or APC thinks it was) so the _once won't load it again.

Conceptually it just makes more sense to me to just use require anyway, since at that point in the code we don't care if its been loaded before, we want to always load it there.

But as to the exact nature of why that fix works, not a clue :)

Working on converting the other update tests in system. So far it's all coming back green.

Lendude’s picture

Assigned: Lendude » Unassigned
FileSize
55.49 KB
72.28 KB

All tests in system/src/Tests/Update converted or deprecated

Status: Needs review » Needs work

The last submitted patch, 26: 2870009-26.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Lendude’s picture

Status: Needs work » Needs review
FileSize
9 KB
78.39 KB

This should clear up some fails.

stripped the part about unsetting exception messages out of \Drupal\Tests\system\Functional\Update\UpdatePathWithBrokenRoutingTest since it's not needed anymore.

The rest is pretty straight forward.

Status: Needs review » Needs work

The last submitted patch, 28: 2870009-28.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Lendude’s picture

Status: Needs work » Needs review
FileSize
2.86 KB
79.2 KB

Going for green....

dawehner’s picture

  1. +++ b/core/modules/system/src/Tests/Update/DbUpdatesTrait.php
    @@ -10,6 +10,9 @@
      * pending db updates through the Update UI.
      *
      * This should be used only by classes extending \Drupal\simpletest\WebTestBase.
    + *
    + * @deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0.
    + *   Use \Drupal\FunctionalTests\Update\DbUpdatesTrait.
      */
     trait DbUpdatesTrait {
    
    +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
    @@ -34,6 +34,9 @@
      *
      * @ingroup update_api
      *
    + * @deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0.
    + *   Use \Drupal\FunctionalTests\Update\UpdatePathTestBase.
    + *
      * @see hook_update_N()
      */
     abstract class UpdatePathTestBase extends WebTestBase {
    

    We need the trigger_error statements, don't we?

  2. +++ b/core/modules/system/tests/src/Functional/Update/UpdatePathWithBrokenRoutingTest.php
    @@ -30,16 +32,6 @@ public function testWithBrokenRouting() {
    -    // The exceptions are expected. Do not interpret them as a test failure.
    -    // Not using File API; a potential error must trigger a PHP warning.
    -    unlink(\Drupal::root() . '/' . $this->siteDirectory . '/error.log');
    -    foreach ($this->assertions as $key => $assertion) {
    -      if (strpos($assertion['message'], 'core/modules/system/tests/modules/update_script_test/src/PathProcessor/BrokenInboundPathProcessor.php') !== FALSE) {
    -        unset($this->assertions[$key]);
    -        $this->deleteAssert($assertion['message_id']);
    -      }
    -    }
    -
    

    Is there a reason you removed this?

  3. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
    @@ -157,35 +173,22 @@ protected function setUp() {
    -    $this->restoreBatch();
    +    // $this->restoreBatch();
     
    

    We should we do with those?

Lendude’s picture

@dawehner thanks for the feedback as always

#31.1 yup, and apparently we need a CR for those deprecations now too, so added that, plus a see to the CR
#31.2 It served no purpose in BrowserTestBase. It seems that was needed to get WebTestBase to pass, but BrowserTestBase passes just fine, and that logic doesn't test anything, it just unsets stuff, so we don't lose any coverage I think, just a WebTestBase specific workaround.
#31.3 Well we need to lose the commented line :). That stuff seems to be for making Simpletest UI play nice with the Batch API. We don't have a UI at the moment, so lets get rid of that line.

dawehner’s picture

I had a look at the original issue #2498625-12: Write tests that ensure hook_update_N is properly run which introduced the restoreBatch call. Sadly there isn't any real explanation. I send @jhedstrom a message instead.

jibran’s picture

Thanks, @dawehner and @Lendude for working on this. Created #2896755: Convert Update Tests to BTB from WTB for DER and it works great.

dawehner’s picture

Thank you @jibran. This is really good to know that contrib modules can leverage it properly. Thank you for letting us know

naveenvalecha’s picture

Nice work guys @dawehner and @lendude.
@jibran,
Nice work on converting contributed projects. Really appreciate your efforts.
Sorry, I don't want to become the guy who brings bad news but Drupal ci is not able to able test those b/c Drupal\FunctionalTests\Update\UpdatePathTestBase does not exist in the 8.4.x head yet I don't know why the CI showed its green. If you'll look at the full output at dispatcher.drupalci.org it will show the error https://dispatcher.drupalci.org/job/drupal8_contrib_patches/9770/console...

----------------   Starting simpletest   ----------------
Directory created at /var/lib/drupalci/workspace/jenkins-drupal8_contrib_patches-9770/ancillary/simpletest.js
mkdir -p /var/www/html/sites/simpletest/xml
ln -s /var/www/html /var/www/html/checkout
chown -fR www-data:www-data /var/www/html/sites
chmod 0777 /var/lib/drupalci/artifacts
chmod 0777 /tmp
sudo -u www-data php /var/www/html/core/scripts/run-tests.sh --list > /var/lib/drupalci/workdir/simpletest.js/testgroups.txt
Directory created at /var/lib/drupalci/workspace/jenkins-drupal8_contrib_patches-9770/artifacts/simpletest.js
cd /var/www/html && sudo -u www-data php /var/www/html/core/scripts/run-tests.sh --color --keep-results --concurrency "1" --types "PHPUnit-FunctionalJavascript" --url "http://localhost/checkout" --sqlite "/var/lib/drupalci/workdir/simpletest.js/simpletest.sqlite" --dburl "mysql://drupaltestbot:drupaltestbotpw@172.17.0.3/jenkins_drupal8_contrib_patches_9770" --directory modules/contrib/dynamic_entity_reference
PHP Fatal error:  Class 'Drupal\FunctionalTests\Update\UpdatePathTestBase' not found in /var/www/html/modules/contrib/dynamic_entity_reference/tests/src/Functional/Update/DerUpdateTest.php on line 15

Fatal error: Class 'Drupal\FunctionalTests\Update\UpdatePathTestBase' not found in /var/www/html/modules/contrib/dynamic_entity_reference/tests/src/Functional/Update/DerUpdateTest.php on line 15
Attempting to connect to database server.

As you have converted dynamic entity reference tests here #2896755: Convert Update Tests to BTB from WTB and Drupal ci also isn't able to test it properly. Would you apply this patch to your Drupal core on your local and also convert the DER tests and try to run it on your local and verify it.

Steps:
- Git clone 8.4.x Drupal core
- Apply the patch here #2870009: Update: Convert system functional tests to phpunit to Core
- Git clone DER in modules directory
- Apply the patch here #2896755: Convert Update Tests to BTB from WTB to DER
- Run the DER tests locally

P.S.: I'm busy with my exams so don't have much time to convert updates tests for any of my contributed modules and test this patch.

//Naveen

jibran’s picture

@naveenvalecha of course, I know this is not committed yet. I'm not concerned with CI yet. I tested it locally after applying the patch and it was green that's why I reported it worked.