Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem
- A patch depends on a specific version of Drupal core.
- A patch might change the installer environment/parameters/conditions.
- PIFR not only depends on Drupal core, it additionally depends on Drush to support the specific version of Drupal core.
- As an external project, Drush naturally should not be updated BEFORE Drupal core has changed.
- → Circular dependency hell.
Objective
- Remove the dependency on Drush entirely from PIFR's patch testing process.
Examples
Comment | File | Size | Author |
---|---|---|---|
#53 | 52-53-interdiff.patch | 2.48 KB | alexpott |
#53 | 2206501.53.patch | 8.64 KB | alexpott |
#52 | 2206501.52.patch | 7 KB | alexpott |
#52 | 47-52.patch | 2.17 KB | alexpott |
#47 | 5-47-interdiff.txt | 3.4 KB | alexpott |
Comments
Comment #1
sun#2210197: Remove public access to Extension::$type, ::$name, ::$filename is yet another victim.
Comment #2
jthorson CreditAttribution: jthorson commentedDrush 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.
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedIdeally, core would provide a dependency-free way of installing itself.
Comment #4
sunI 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.
Comment #5
sunCombined with #1808220: Remove run-tests.sh dependency on existing/installed parent site
...this should actually work...
Comment #6
sun#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:
Comment #7
sunAny 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... :-/
Comment #8
ParisLiakos CreditAttribution: ParisLiakos commentedyes please. also in this issue we are forced to keep a lot of code around so that drush does not break
Comment #9
rfayFYI, 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.
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commentedThis 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!.
Comment #11
rfayI'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.
Comment #12
sun@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! :-)
Comment #13
sunBump. — This blocks several core issues that have to land before beta. Time is running out.
Comment #14
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #15
sunTo 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."
Comment #16
ricardoamaro CreditAttribution: ricardoamaro commentedI'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 ))
Comment #17
rfay@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.
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedDocker 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 :)
Comment #19
jthorson CreditAttribution: jthorson commentedDocker 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.
Comment #21
rfay@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.
Comment #23
alexpottSo this broke the following tests:
Postponing until these issues are fixed.
Comment #24
sunFor the last bullet: #2248985: ScriptTest fails on Windows, runs against parent site
Comment #25
sunQuoting #23:
Drupal\simpletest\Tests\SimpleTestTest
- needs proof that it works before committing this patch again.Drupal\system\Tests\System\ScriptTest
- #2261477: Remove broken Drupal\system\Tests\ScriptTestOnly (2) remains, needs manual testing.
Comment #26
jthorson CreditAttribution: jthorson commentedAre you suggesting that there has been a fix for #2, and we are ready for that manual testing?
Comment #27
sunI 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.
Comment #28
sunSimpleTestTest
is executed correctly, so we should be ready to move forward.Comment #29
sunPatch in #5 still applies cleanly.
Comment #30
sun@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.
Comment #31
moshe weitzman CreditAttribution: moshe weitzman commentedI confirm that Drupal\simpletest\Tests\SimpleTestTest is passing when used with --sqlite.
Comment #32
YesCT CreditAttribution: YesCT commentedthis might be closed (wont fix) in favor or #2386743: Deploy DrupalCI 2.0
Comment #33
tim.plunkettBump, #2372507: Remove _system_path from $request->attributes is another issue that is blocked on drush.
Comment #34
alexpottPatch still applies and we still have issues with dependencies on Drush eg. #2421005: Add \Drupal::hasContainer() instead of checking if \Drupal::getContainer() === NULL
Comment #35
basic CreditAttribution: basic commentedAfter 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.
Comment #37
basic CreditAttribution: basic commentedThis is now running on all active testbot workers
Comment #38
webchickYAY! Thanks so much!
Comment #40
basic CreditAttribution: basic commentedThis 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.
Comment #41
YesCT CreditAttribution: YesCT commentedhm. that didn't go to the testbot.
I mentioned it on #2420915: patches on issues not being sent to testbot for testing
Comment #42
webchickNot sure if PIFR is a project that has testbot enabled for it (irony of ironies :D))
Comment #43
YesCT CreditAttribution: YesCT commentedoh... sorry, I thought I saw an earlier patch in here green. but I guess not.
Comment #44
alexpottI'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?
Comment #45
alexpottComment #46
alexpottI'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.
Comment #47
alexpottSo 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:
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.
Comment #48
alexpottSo 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.
Comment #49
alexpottSo the blocker has landed. Any chance we can try out #47?
Comment #50
basic CreditAttribution: basic commented@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.
Comment #51
alexpott@basic well your mistake allowed me to find out what was going on - so providence was with us!
Comment #52
alexpottA 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.
Comment #53
alexpottWhilst 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.
Comment #54
basic CreditAttribution: basic commentedConfirming 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.
Comment #56
basic CreditAttribution: basic commentedCommitted, will be slowly rolling to bots
Comment #58
basic CreditAttribution: basic commentedThe change is now live on all testbot workers