Problem

  1. A patch depends on a specific version of Drupal core.
  2. A patch might change the installer environment/parameters/conditions.
  3. PIFR not only depends on Drupal core, it additionally depends on Drush to support the specific version of Drupal core.
  4. As an external project, Drush naturally should not be updated BEFORE Drupal core has changed.
  5. → Circular dependency hell.

Objective

  • Remove the dependency on Drush entirely from PIFR's patch testing process.

Examples

  1. #2176621: Remove global $databases
  2. #1757536: Move settings.php to /settings directory, fold sites.php into settings.php
  3. #2210197: Remove public access to Extension::$type, ::$name, ::$filename
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

jthorson’s picture

Drush was added in #2148535: Use drush for the install process, to work around PHP 5.4 issues in #2147053: Simpletest changes to make php5.4 work with PIFR.

We could potentially roll back to a patch in the second link above, but I think Randy (who did the drush conversion) and I will need to consult. I'm not very eager to roll back to the old way of doing things ... but perhaps there's an option to make the installation process pluggable; using drush for D6/D7/Contrib testing, and the bastardized simpletest browser fake-click-through method for D8 core.

Damien Tournoud’s picture

Ideally, core would provide a dependency-free way of installing itself.

sun’s picture

I just recalled that I worked on getting rid of the entire dependency of a parent site/test runner for run-tests.sh in the past already:

#1808220: Remove run-tests.sh dependency on existing/installed parent site

So in light of these problems, I'm working on a new patch for that right now.

Because with that, we can simply skip the entire "install parent site + enable simpletest module" step of PIFR. Of course, only for D8. But that's not a problem, because the setup code contains plenty of conditions on the Drupal major version already.

sun’s picture

Status: Active » Needs review
FileSize
3.31 KB
sun’s picture

#1808220: Remove run-tests.sh dependency on existing/installed parent site just landed.

Theoretically this patch should work as-is. Perhaps the following parameters need to be tweaked though:

+++ b/review/simpletest/pifr_simpletest.client.inc
@@ -114,6 +115,8 @@
+      $command .= ' --sqlite /tmpfs/pifr/test.sqlite';
+      $command .= ' --dburl ' . $db_url['pifr_checkout'];
sun’s picture

Any chance to move forward here?

I just had to implant another fancy trick to support Drush in #1757536: Move settings.php to /settings directory, fold sites.php into settings.php... :-/

ParisLiakos’s picture

yes please. also in this issue we are forced to keep a lot of code around so that drush does not break

rfay’s picture

FYI, we use a fixed version of drush (a specific commit). We'll be happy to change the commit when necessary.

You should be aware that the older pre-drush-si technique required a horrible traverse of the install process, so every time there was a simple change anywhere in the install wizard the whole thing fell apart. That's why we are where we are. As I recall I could figure out *no* way to get the installer-walkthrough technique to work with php5.4. I didn't figure out why.

moshe weitzman’s picture

This looks like a lovely, simple patch. Hopefully a bot maintainer can test it soon.

Yes please lets do this. All this talk about "but we have to remain Drush compatible" makes it sound like Drush is dragging down D8 development. Far from it!.

rfay’s picture

I'd be interested to know if anybody has tested the patch. I don't object, but certainly in this world it's unlikely to work if it hasn't actually been tried on a testbot.

sun’s picture

@rfay: In case you missed it, #1808220: Remove run-tests.sh dependency on existing/installed parent site eliminated the requirement of a "parent site" for running tests altogether for D8. :-)

This issue is about performing the last remaining step of that change: Actually using the new facility on PIFR/testbots.

To preemptively combat concerns:

The interactive installer is natively tested by the Drupal core test suite itself in D8 now. #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead enabled testing of the interactive installer (and much more) by turning each test into an actual Drupal [multi-]site directory.

This patch has not been tested on an actual PIFR/testbot environment yet. I have a fair understanding of the PIFR code base, but the Vagrant stuff for testing a testbot is still beyond my horizon at this point.


@moshe: I don't think anyone is blaming Drush for status quo. The idea of leveraging it in order to get a test runner site up & running seemingly made sense in the past. Drush is great, we all love Drush! :-)

sun’s picture

Bump. — This blocks several core issues that have to land before beta. Time is running out.

moshe weitzman’s picture

I actually have the The new Docker testbot running on my machine and it is awesome. So I'd be happy to test this but I think that the code being patched here lives outside of the scope of the docker system. So are pifr_drupal.client.inc and pifr_simpletest.client.inc going away? I'm not too clear on the code flow.

sun’s picture

To my knowledge, the new Docker code actively uses the new run-tests.sh already.

However, I've also been told that the new Docker implementation is not ready for prime time yet + might still take a couple of weeks to get deployed to the qa.d.o infrastructure. — But the beta is also planned "in a few weeks."

ricardoamaro’s picture

I've been talking to @moshe weitzman and i'm doing a patch.
This enables the docker testbot to skip drush install and just run the new run-tests.sh if (( $DRUPALVERSION >= 8 ))

rfay’s picture

@ricardoamaro I'd be happy for you to take the lead on testing this and getting it in. I'm sure @jthorson would be OK with that too.

moshe weitzman’s picture

Docker now skips install of parent Drupal for D8.

I don't think ricardoamaro or me or Sun are easily able to move this issue along as it requires setting up a pifr test environment.

I'd love to see an outline of what's needed to get that live. Is that documented anywhere? Shall we have a Hangout about it? I'd like to stop hijacking this issue :)

jthorson’s picture

Docker is a ways away ... we have a test runner, but the rest of the infra still needs to be developed. We're currently trying to schedule a planning meeting across four continents. If you want to be added to that invite, let me know ... but as a planning meeting, don't expect too many answers. From a timeline perspective, I don't expect to have a lot of dedicated time until the second week of May (given the current view of both vacation and kids schedules).

As for PIFR, this is high on the priority list, but likewise, I probably will not be getting to it before the first week of May at best, and second week at worst.

Come that second week of May, I'm at home alone with no kid interruptions, and anticipate being able to spend a large chunk of time catching up.

That said, visually I'm okay with the patch, if someone else wants to throw it on our staging/dev testbots and give it a thorough testing before I get to it.

  • Commit ea0eef5 on 6.x-3.x by jthorson:
    [#2206501] by sun: Remove dependency on Drush for D8 tests
    
rfay’s picture

@moshe weitzman "I don't think ricardoamaro or me or Sun are easily able to move this issue along as it requires setting up a pifr test environment."

https://drupal.org/automated-testing is where this stuff is kept mostly.
Last item on there is Run A Drupal.org Testbot Under Vagrant

It's not at all hard to do. Fine for development.

  • Commit 90a8e1b on 6.x-3.x by rfay:
    Revert "[#2206501] by sun: Remove dependency on Drush for D8 tests"...
alexpott’s picture

Status: Needs review » Postponed

So this broke the following tests:

Postponing until these issues are fixed.

sun’s picture

sun’s picture

Status: Postponed » Needs review
Issue tags: +Needs manual testing

Quoting #23:

  1. Broke all phpunit: Fixed by #2248845: [run-tests.sh] simpletest_phpunit_xml_filepath() depends on public:// stream wrapper
  2. Drupal\simpletest\Tests\SimpleTestTest - needs proof that it works before committing this patch again.
  3. Drupal\system\Tests\System\ScriptTest - #2261477: Remove broken Drupal\system\Tests\ScriptTest

Only (2) remains, needs manual testing.

jthorson’s picture

Are you suggesting that there has been a fix for #2, and we are ready for that manual testing?

sun’s picture

I can't remember why Drupal\simpletest\Tests\SimpleTestTest has been listed as a potential problem here. Only #1 and #3 presented known problems to me, but those are fixed now.

I'll test locally (on Windows). If we can additionally test manually on a testbot, that would be great.

sun’s picture

SimpleTestTest is executed correctly, so we should be ready to move forward.

$ php core/scripts/run-tests.sh --url http://drupal8.test/ --sqlite test/test.sqlite --dburl sqlite://localhost/test/db.sqlite --class Drupal\simpletest\Tests\SimpleTestTest

Drupal test run
---------------

Tests to be run:
  - Drupal\simpletest\Tests\SimpleTestTest

Test run started:
  Monday, September 1, 2014 - 15:19

Test summary
------------

Drupal\simpletest\Tests\SimpleTestTest                        51 passes

Test run duration: 1 min 22 sec
sun’s picture

Patch in #5 still applies cleanly.

sun’s picture

@moshe weitzman discovered #2332071: Custom port in run-tests.sh --dburl is not parsed in manual testing.

However, testbots are using standard ports, so that shouldn't block this issue.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

I confirm that Drupal\simpletest\Tests\SimpleTestTest is passing when used with --sqlite.

~/htd/d8 (8.0.x *<)$ php core/scripts/run-tests.sh --url http://d8.local:8083 --sqlite test/test.sqlite --dburl "mysql://root:@127.0.0.1:33067/d8"  --class "Drupal\simpletest\Tests\SimpleTestTest"

Drupal test run
---------------

Tests to be run:
  - Drupal\simpletest\Tests\SimpleTestTest

Test run started:
  Wednesday, September 3, 2014 - 13:51

Test summary
------------

Drupal\simpletest\Tests\SimpleTestTest                        51 passes                                      
YesCT’s picture

this might be closed (wont fix) in favor or #2386743: Deploy DrupalCI 2.0

tim.plunkett’s picture

Bump, #2372507: Remove _system_path from $request->attributes is another issue that is blocked on drush.

alexpott’s picture

Patch still applies and we still have issues with dependencies on Drush eg. #2421005: Add \Drupal::hasContainer() instead of checking if \Drupal::getContainer() === NULL

basic’s picture

After reading through this, it sounds like the patch from #5 works with local testing and needs to be tested on a production testbot worker and then can be pushed to all testbots.

  • basic committed b44c774 on 6.x-3.x
    [#2206501] by sun: Remove dependency on Drush from test reviews
    
basic’s picture

Status: Reviewed & tested by the community » Fixed

This is now running on all active testbot workers

webchick’s picture

YAY! Thanks so much!

  • basic committed 0b470da on
    Revert "[#2206501] by sun: Remove dependency on Drush from test reviews...
basic’s picture

Status: Fixed » Needs review
FileSize
4.51 KB

This was causing errors with the lack of --dburl in the PIFR_CLIENT_PHP tests. @berdir helped track down the line and I've added a quick untested update of what I believe needs to happen.

YesCT’s picture

hm. that didn't go to the testbot.
I mentioned it on #2420915: patches on issues not being sent to testbot for testing

webchick’s picture

Not sure if PIFR is a project that has testbot enabled for it (irony of ironies :D))

YesCT’s picture

oh... sorry, I thought I saw an earlier patch in here green. but I guess not.

alexpott’s picture

I've created #2421335: Using run-tests.sh with the --list option should not require a db (of any kind) to fix this in a better way - so we can continue to go with #5. Third time lucky?

alexpott’s picture

alexpott’s picture

I've reviewed the fix in #40 and it looks like it would fix the problem - nice work @basic - but the fix in #2421335: Using run-tests.sh with the --list option should not require a db (of any kind) is a better fix - will try to get it committed today.

alexpott’s picture

FileSize
6.48 KB
3.4 KB

So there is one more thing that is failing. It appears that the rollback of this patch did not occur on every testbot - although results are confusing. See https://qa.drupal.org/pifr/test/969063

The relevant part is:

[03:38:16] Command [/usr/bin/php ./core/scripts/run-tests.sh --php /usr/bin/php --url http://ip-172-30-0-64/checkout --all --list 2>&1] succeeded.
[03:38:16] Review complete. test_id=969063 result code=8 details=Array
(
    [@reason] => tests were executed, but no results were found
)

Basically pifr_client_review_pifr_simpletest::get_results() is connecting to the mysql db and looking for the results. And because we've stored them in an sqlite db there are none. Patch attached fixes that.

The interdiff is back to the patch in #5.

alexpott’s picture

So whilst working on #2421335: Using run-tests.sh with the --list option should not require a db (of any kind) @berdir and I discovered that #2753 has not had this patch rolled back for some reason. Lucky thing too as this allowed me to identify the problem fixed in #47. Perhaps we can somehow use that bot (which has been removed from the pool by @berdir) to prove that we've finally got this done. The code in #47 is not the most elegant but it is in line how the rest of the code is working so I think it is acceptable.

alexpott’s picture

So the blocker has landed. Any chance we can try out #47?

basic’s picture

@alexpott: That is my mistake, I thought I disabled bot 2753 from accepting patches from qa.d.o. I had applied the patch from #39 there for testing and found issues, but was planning to follow up today to troubleshoot them. Glad to see #47 was put together.

I have applied the patch from #47 on bot #2753 for testing. Hopefully this resolves the issue.

alexpott’s picture

@basic well your mistake allowed me to find out what was going on - so providence was with us!

alexpott’s picture

FileSize
2.17 KB
7 KB

A slightly improved patch based on testing of the code I added. I'm not 100% that it is fixing a bug but it definitely means we have less dependencies.

alexpott’s picture

FileSize
8.64 KB
2.48 KB

Whilst testing with @basic we found some problems with the includes. I also noticed that we'd have a static method that counts the assertions whilst tests are in progress.

basic’s picture

Confirming that we tested this on a production-level EC2 testbot instance. Local tests passed, and testbot communication to/from qa.drupal.org worked as expected. We're planning to roll out #53 on Monday, one bot at a time.

  • basic committed 5da6faa on 6.x-3.x authored by alexpott
    Issue #2206501 by alexpott, sun, basic: Remove dependency on Drush from...
basic’s picture

Status: Needs review » Reviewed & tested by the community

Committed, will be slowly rolling to bots

  • basic committed 2145cc8 on 6.x-3.x authored by alexpott
    Issue #2206501 by alexpott, sun, basic: Remove dependency on Drush from...
basic’s picture

Priority: Critical » Normal
Status: Reviewed & tested by the community » Fixed

The change is now live on all testbot workers

Status: Fixed » Closed (fixed)

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