Problem/Motivation
Numerous test failures occurred across multiple branches today, mostly with fails like:
#slow.Drupal\BuildTests\Composer\Template\ComposerProjectTemplatesTest
✗
Drupal\BuildTests\Composer\Template\ComposerProjectTemplatesTest
exception: [Other] Line 0 of sites/default/files/simpletest/phpunit-1.xml:
PHPunit Test failed to complete; Error: PHPUnit 8.5.2 by Sebastian Bergmann and contributors.
Testing Drupal\BuildTests\Composer\Template\ComposerProjectTemplatesTest
.E. 3 / 3 (100%)
Time: 9.58 minutes, Memory: 14.50 MB
There was 1 error:
1) Drupal\BuildTests\Composer\Template\ComposerProjectTemplatesTest::testTemplateCreateProject with data set "recommended-project" ('drupal/recommended-project', 'composer/Template/RecommendedProject', '/web')
Symfony\Component\Process\Exception\ProcessTimedOutException: The process "COMPOSER_CORE_VERSION=9.0.x-dev composer create-project drupal/recommended-project testproject 9.0.x-dev -s dev -vv --repository /tmp/build_workspace_0cb74fdd1a5642e0cdf9b32245d4a3a15qTmwc/test_repository/packages.json" exceeded the timeout of 300 seconds.
/var/www/html/vendor/symfony/process/Process.php:1253
/var/www/html/vendor/symfony/process/Process.php:417
/var/www/html/vendor/symfony/process/Process.php:238
/var/www/html/core/tests/Drupal/BuildTests/Framework/BuildTestBase.php:326
/var/www/html/core/tests/Drupal/BuildTests/Composer/Template/ComposerProjectTemplatesTest.php:131
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:691
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:621
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:200
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:159
ERRORS!
Tests: 3, Assertions: 32, Errors: 1.
Since it mentions a 5-minute (!) timeout, I wondered if the spate of fails was related to an infrastructural issue (on or outside of our internal infra). @Mixologic looked into it and said:
but as to why that test is slow is beyond me. It should be all using local path repos and be very quick.
looks like its inside of they symfony process as the timeout:
Symfony\Component\Process\Exception\ProcessTimedOutException
It could just be that aws suffered some instability around when those were processed.
yeah, Im concerned that the timeouts would happen though. Those tests are /should be not using the internet at all.
Proposed resolution
Determine whether ComposerProjectTemplatesTest
relies on an external network connection, and if it does, determine whether we can fix it to not do that.
Remaining tasks
TBD
User interface changes
N/A
API changes
TBD
Data model changes
N/A
Release notes snippet
TBD
Comment | File | Size | Author |
---|---|---|---|
#37 | 3123933-8.8.x-34.patch | 6.18 KB | alexpott |
#35 | 3123933-35.patch | 6.42 KB | alexpott |
#35 | 34-35-interdiff.txt | 1.01 KB | alexpott |
#34 | 3123933-8.8.x-34.patch | 6.18 KB | longwave |
#28 | 3123933-8.9.x-and-9.0.x-28.patch | 6.93 KB | greg.1.anderson |
Comments
Comment #2
longwaveConfirmed locally with tcpdump that ComposerProjectTemplatesTest generates network traffic to packages.drupal.org and repo.packagist.org even if the Composer cache is primed.
This can be verified by logging the verbose output from the
composer create-project
command which includes lines such as:If the Composer cache is wiped before starting the test, then the full set of package metadata and the packages themselves are all downloaded.
Comment #3
alexpottThis is being caused by the version constraints in composer/Template/RecommendedProject/composer.json which are:
There's now 8.9.x-dev available so composer tries to install that.
Here's the actual error output and not the binary string reported in the error logs:
Comment #4
alexpottThe test failure is no longer random - https://www.drupal.org/pift-ci-job/1677969
Comment #5
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI encountered the binary string problem in #3134606: [backport] PHP Fatal error in 8.9.x / 9.0.x upgrade path. I started getting correct results again when I switched to using PHP's native
exec
function instead of symfony/process.Comment #6
alexpottThe binary string thing happens because PHPUnit incorrectly determines the thing to be a binary string. It's not but there are characters in the string that makes it think it is a binary string. It's not actually part of the problem - it just adds a layer of obfuscation to the problem.
Comment #7
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented#6: Yes, exactly. By "correct results" I mean that my failing tests were failing with readable messages instead of unreadable (but transliterable) hexadecimal sequences. This is something else that only recently started happening.
Comment #8
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI remember working on this test a while back. I discovered that if the dependencies of the template project were also added to the packages.json file, that the entire composer create-project could run without Packagist.
I don't remember what I was working on at the time. My test might be in an unfinished patch, or it might just be lost.
Comment #9
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThe issue here is that even though the Drupal projects in our repo are loaded via a path repo rather than going through Packagist, we still hit Packagist for all of our dependencies of dependencies.
Here's a failing patch that demonstrates the problem by disabling Packagist for the test. Any package that we can't get locally will fail to be found, so this build fails due to unresolvable dependencies. I've also made an attempt here to define packages for all of our dependencies. I briefly had this working in ad-hoc testing locally, but the version in code didn't work, and now I can't reproduce my earlier ad-hoc results. I'll continue to try different permutations to see which ways Composer will let us define local packages.
Comment #10
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedSo I did notice that path should be
$path = "$root/vendor/$name";
, but fixing that didn't help Composer find the vendor projects. Perhaps putting the vendor projects directly in a "package" repository in the template project will work better.Comment #11
xjmOnce we have a test it's NW. :) Also queued it to expose the fail.
Comment #12
xjmIt's back to being random, at least for HEAD.
Comment #13
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented@mglaman got local package repositories working: https://twitter.com/nmdmatt/status/1260760241135853570
Comment #14
alexpottThis test and it's reliance on the internet causes problems for releases.
If I change the \Druapal::VERSION locally to 8.6.6 it fails locally for me. It looks like the symlinking thing doesn’t happen. The output contains
Extracting archive - Installing drupal/core-dev (8.8.6)\n
but the test expects it do have: Symlinking from PATH/TO/DIRECTORY
appended. I don’t know why this is different from when you run the test with 8.6.6-dev as the version. But when my version is 8.6.6-dev the output isExtracting archive - Installing drupal/core (8.8.x-dev): Symlinking from /private/tmp/build_workspace_223b32aed9d1a85d8232daac9f6100853PxoqE/core
.I suspect this is because packagist & the repo know nothing about the 8.6.6 tag. But who knows?
Changing the \Drupal:VERSION to 8.6.7-dev and the test passes again.
Comment #15
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI got the internet-free version of this working yesterday; I just need to clean it up and make a patch.
Comment #16
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedEDIT: This comment is all madness and rambling. Please jump ahead to #19.
Here's a new patch that (a) doesn't use Packagist or other internet resources AND (b) passed BUT the test modifies files in the source tree when it runs.
Once I got this green I spent some time last night moving the fixtures around in the hopes that I could make it modify the test fixtures rather than the original source AND still work, but the Composer path repositories setup in this test are a fragile house of cards. It's kind of difficult to modify the configuration, because in some instances, Composer will only allow relative paths, but when I tried to consistently use a "package" repository (where absolute paths are allowed), I ran into the other Composer limitation where you run into trouble if you define more than twenty or so path repositories (yep, if there are too many path repositories, Composer stops working). Relative paths are tricky, because it's not always clear what they are supposed to be relative to, and so sometimes making minor adjustments that seem equivalent end up breaking everything.
Hopefully this testbot will come back with passing tests, like my local runs did. Once we have a baseline here that works, we can make incremental changes until we have something that works and that we can live with.
Comment #17
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedComment #18
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThe testbot won't allow our bad practices, and gives us a "permission denied" when we try to alter the sources we are testing. The modification of the template projects themselves shouldn't be too hard to fix; however, there is a more difficult problem. This patch avoids using Packagist in tests by creating a local "Composer"-type package repository from all of the packages in the vendor directory. Vendor is used in-place to avoid copying it. It seems that "Composer"-type package repositories only work with relative paths. I tried using a "Packages"-type package repository, but as I mentioned in #16, this ran up against the limit on how many path repositories Composer will allow you to use. (Apparently, "Composer"-type packages only count as one, but "Package"-type repositories count as one per package.) We can probably get around this by copying the vendor directory to another location, but I was hoping to avoid doing that. I also considered symlinking vendor, but avoided that idea to avoid problems when running the tests on Windows.
Comment #19
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedOK Let's just ignore #16, I was too tired when I was working on this last night. Here's a cleaned-up version with an interdiff with #9.
We avoid using Packagist while running this test by building a package repository for everything in the vendor directory.
Comment #20
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedCode style and a little cleanup / simplification.
Comment #21
alexpottI can confirm that #20 works when there's no access to the internet. Nice.
Also if
\Drupal:VERSION
is something like9.1.1
and composer/Metapackage/CoreRecommended/composer.json and composer/Metapackage/PinnedDevDependencies/composer.json have been updated too then the test is passing. Double nice!"We won't worry about this for now" should be an @todo pointing to an issue.
Given that we're futzing with the repositories of the composer.json under test isn't doing an override just for that composer.json okay - i.e. https://getcomposer.org/doc/05-repositories.md#disabling-packagist-org
Or does this not work because we're doing composer create-project. And if this is the case we should probably document that.
Should we unset all repositories here. In the unlikely case that we add another one to our templates this would make it less likely that the test would become internet dependent again.
So the only real change here is the
-n
for no interaction. If this is necessary can we use--no-interaction
. It helps to have to think less what everything means.As above for the -n
Build an array called $packages from an array called $package does not make for the most readable code. How about
$installed as $project
Is there any reason you preferred this over the root composer.lock file. It's interesting to ponder which format is less likely to change here. I wonder if we should use the composer command
composer show --installed --format=json
here instead. As that's probably a more guaranteed API - rather than relying on Composer keeping it's file formats the same.$full_path
$project_version or $version
The is_dir() is interesting here - when is that not true - I guess this excludes drupal/core :) - perhaps worth a commment.
Comment #22
longwaveNice work!
NW for #21 and the below
$composerHome -> $composer_home
Do we need
here?
Do we need
here?
existance -> existence
hapened -> happened
Comment #23
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThanks for the reviews. This was Harder Than Your Average Patch thanks to Composer oddities.
#21:
1. Added a TODO with a reference to #3128631: Update dependencies composer/composer ^2 and composer/semver to ^3
2. Yes, this is needed for create-project. Added a comment.
3. It's not possible to do this with`composer config --unset`, so I changed the code to read in and rewrite composer.json instead.
4 & 5. Spelled out --no-interaction and --global options.
6 & 8 - 9. Fixed code style.
7. I was reading installed.json to get the project type; however, it turned out that including the project type in the package repository was not only unnecessary, but also harmful. Since I don't need the type any longer, I switched to using `composer info --format=json`, which includes the information needed to build the repository structure.
10. We are building a set of packages with path repositories pointing to things in vendor, so it naturally makes sense to check to see if the project exists in vendor before pointing at a directory that does not exist.
#22:
1. Fixed variable code style (and $composer_version_line too)
2 & 3. Added failure checks for prudence.
4. Fixed spelling
Comment #24
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedComment #25
longwaveOnly some real nitpicks left as far as I can see:
This should follow coding standards, ie. something like
Out of scope, but I wonder if all the Composer tests should just set the COMPOSER_NO_INTERACTION environment variable and then we don't have to remember to sprinkle --no-interaction everywhere.
we're -> we
Unnecessary string interpolation? Took me a few attempts to mentally parse this :)
Comment #26
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAddressed #25.
I don't have a strong opinion on using COMPOSER_NO_INTERACTION. I guess it would make for DRY-er tests, but I kind of like being explicit in tests.
Comment #27
longwaveThanks! All points addressed, over to core committers for another look.
Comment #28
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedOne last code standards fix (DOH!).
The 9.0.x patch seems to apply to 8.9.x; let's try running it both places.
Comment #29
alexpottCommitted and pushed a6eaea1b96 to 9.1.x and e1ab4c0d19 to 9.0.x and 09f6211187 to 8.9.x. Thanks!
We need to backport this to 8.8.x too because at the moment without this fix the commit with the release version will fail tests.
Comment #33
alexpottComment #34
longwaveRerolled, but this failed locally, and I don't have time to fix it now. Looks like makeVendorPackage() needs to take -dev releases into account.
[RuntimeException]
Could not load package behat/mink-selenium2-driver in file:///tmp/build_workspace_0ee39e33a2eeb27e75dc5340ec6269baGuJ3i3/vendor_packages/packages.json: [UnexpectedValueException] Invalid version string "1.3.x-dev 0a09c43"
[UnexpectedValueException]
Invalid version string "1.3.x-dev 0a09c43"
Comment #35
alexpottHere's a fix for #34. It's happening because we've not backported #3078671: Pin behat/mink and behat/mink-selenium2-driver to use resolvable release so we have to deal with funky dev versions.
Comment #37
alexpott#3078671: Pin behat/mink and behat/mink-selenium2-driver to use resolvable release got backported. #34 now passes on 8.8.x so re-uploading so it's the last patch.
Comment #39
alexpottCommitted a2e22ea and pushed to 8.8.x. Thanks!
@longwave thanks for the quick work on the 8.8.x patch.