Update: Convert system functional tests to phpunit

I could apply the patch, so no reroll is needed

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

@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.

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

These tests pass locally on both SQlite and MySQL :(

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.

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.

require vs require_once ....

This now comes back green locally with APC switched on.

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.

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

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.

Going for green....

@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.

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

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

@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.

The addition of the restoreBatch method was made in #2498625: Write tests that ensure hook_update_N is properly run because the update test base classes needed to call things that were previously just hardcoded in WebTestBase::setUp() in a slightly different order (it has been quite some time though, so this is just my best recollection).

@jhedstrom thanks for the feedback!

so that would lead me to the conclusion that we can just lose restoreBatch in BrowserTestBase if the tests pass without it (which they do).

Patch looks good. All the feedback is addressed. Contrib can use new base class as well as shown in #34. This is ready imo.

1. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
   @@ -157,43 +173,122 @@ protected function setUp() {
   +    // Creates the directory to store browser output in if a file to write
   +    // URLs to has been created by \Drupal\Tests\Listeners\HtmlOutputPrinter.
   +    $browser_output_file = getenv('BROWSERTEST_OUTPUT_FILE');
   +    $this->htmlOutputEnabled = is_file($browser_output_file);
   +    if ($this->htmlOutputEnabled) {
   +      $this->htmlOutputFile = $browser_output_file;
   +      $this->htmlOutputClassName = str_replace("\\", "_", get_called_class());
   +      $this->htmlOutputDirectory = DRUPAL_ROOT . '/sites/simpletest/browser_output';
   +      if (file_prepare_directory($this->htmlOutputDirectory, FILE_CREATE_DIRECTORY) && !file_exists($this->htmlOutputDirectory . '/.htaccess')) {
   +        file_put_contents($this->htmlOutputDirectory . '/.htaccess', "<IfModule mod_expires.c>\nExpiresActive Off\n</IfModule>\n");
   +      }
   +      $this->htmlOutputCounterStorage = $this->htmlOutputDirectory . '/' . $this->htmlOutputClassName . '.counter';
   +      $this->htmlOutputTestId = str_replace('sites/simpletest/', '', $this->siteDirectory);
   +      if (is_file($this->htmlOutputCounterStorage)) {
   +        $this->htmlOutputCounter = max(1, (int) file_get_contents($this->htmlOutputCounterStorage)) + 1;
   +      }
   +    }
   +  }
   

   Can we get a follow-up to get this in its own method, so we don't have to duplicate it between here and BTB?

2. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -262,6 +258,32 @@
   +    $this->root = dirname(dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__))));
   

   nit, should be a space after the -

Committed as 7204a00 and pushed to 8.5.x.

Cherry-picked as 4dc3a46 and pushed to 8.4.x.

Published change record.

Great work here...on with the conversions!

Can we get the follow-up as per #42.1?

@larowlan++++++

Here is the follow up #2904834: Move BROWSERTEST_OUTPUT_FILE logic in BrowserTestBase::setUp to a method and make BrowserTestBase and and \Drupal\FunctionalTests\Update\UpdatePathTestBase use it

#2896755: Convert Update Tests to BTB from WTB came back green after this went in. I think we should update some documentation around this conversion. The namespace/location change is trivial but we have to change the dump location in tests as well.