Install composer dependencies for contrib projects before running tests

Is there a way to get around this in the meantime? E.g. composer_manager as a test dependency?

No, there is not a way to run arbitrary commands at the moment for a job configuration on drupal.org.

On your own drupalci instance, I think it is possible to setup a custom job (YAML configuration) that adds steps to run composer manager's command before running composer install.

DrupalCI currently runs a composer install on core, and will have to even for contrib tests.

If core had a defined behavior for managing the composer-based dependencies of contrib, they'd likely be installed during that existing build phase.

Over in core you can start here: #2002304: [META] Improve Drupal's use of Composer

This issue can also prevent the installation of modules with unmet composer dependencies. In DrupalCI this would at least tell you that we can't run the test: #2494073: Prevent modules which have unmet Composer dependencies from being installed

Here's a patch:

1. This moves the composer step into generic so that it can be run in setup and in install so that...
2. ...the composerinstall preprocess can add composer_manager init script and run drupal-rebuild and update in a different step. This has to be done because the composer_manager init has to run after the initial composer install for 8.1.x and greater.
3. This also depends on the patch in #2664274: Combination of --prefer-dist and .gitattributes confuses our vendor test cleanup.

Here is my config for testing where MODULENAME and MODULENAME_TESTGROUPS should be replaced respectively with the contrib module and its test groups.

DCI_DBVersion: pgsql-9.4
DCI_PHPVersion: 5.6
DCI_TestItem: directory:modules/MODULENAME
DCI_CoreBranch: 8.1.x
DCI_JobType: simpletest
DCI_AdditionalRepositories: git,git://git.drupal.org/project/MODULENAME.git,8.x-1.x,modules/MODULENAME,1;git,git://git.drupal.org/project/composer_manager.git,8.x-1.x,modules/composer_manager,1
DCI_TestGroups: MODULENAME_TESTGROUPS
DCI_ComposerInstall: True
DCI_RunScript: /var/www/html/core/scripts/run-tests.sh
DCI_DBUrl: pgsql://drupaltestbot:drupaltestbotpw@host/drupaltestbot
DCI_Concurrency: 1
DCI_CoreRepository: git://git.drupal.org/project/drupal.git
DCI_GitCheckoutDepth: 1
DCI_RunOptions: sqlite /var/www/html/results/simpletest.sqlite
DCI_Fetch: https://www.drupal.org/files/issues/2664274-52.patch,.
DCI_Patch: 2664274-52.patch,.

Scratch #3 in the comment above. I forgot that a patch was already committed in #2664274: Combination of --prefer-dist and .gitattributes confuses our vendor test cleanup, and so my patch in #6 works fine without it.

Updated issue summary.

Closed #2632912: Support composer_manager as a duplicate.

I'd suggest we use drupal-merge-plugin: https://github.com/paul-m/drupal-merge-plugin/blob/master/README.md

I spoke with @bojanz in IRC and he says composer_manager is likely going to EOL after the next core release. He can correct me if that's wrong...

If we were just building the site's modules we could simply say composer require drupal/contrib-name and be done, but that's not what we're doing.

We have to check out a specific branch, apply a patch which could change the composer.json file, and then build the dependencies.

We could insert a step where we say composer require mile23/drupal-merge-plugin, and then subsequently use composer update to allow it to discover extensions and determine their dependencies.

Ideally this plugin would be part of core, ready for us to use, but that might be a more protracted process than inserting a step here.

I support #9.
Having a plugin do the module dependency merging (for testing purposes) is a more elegant solution than composer_manager.

To make sure I understand, are #9 and #10 focusing only on DrupalCI or will composer_manager become EOL altogether at some point? It feels important to know what are current/upcoming recommended approaches for handling 3rd party contrib vendor dependencies.

@anavarre
Once Drupal 8.0.6 and 8.1-beta2 get released there will be no reason to use composer_manager instead of getting the whole module via Composer.
"composer require drupal/commerce", instead of 1) download Commerce somehow 2) initialize composer_manager 3) run composer drupal-update.
.
#9 is for cases where the module must absolutely be present without being fetched by Composer (== tests).

Makes sense, thanks for the clarification.

If we patch core with something like this, would DrupalCI just work?

@timmillwood
It's not that easy, we can't just add wildcard paths to composer.json hoping it will simulate our extension discovery close enough.

I think this can still be worked on since we're using 8.2.x i.e. not postponed.

This might also mean that the composer step can stay in setup, but it may be a good idea to move it into generic anyway.

I'm having trouble with using merge plugin and it trying to download drupal/. Consistently getting RuntimeException similar to

>> Mile23\DrupalMerge\MergePlugin_composer_tmp2::onPostPackageInstall
> Drupal\Core\Composer\Composer::vendorTestCodeCleanup
  - Installing drupal/commerce (dev-8.x-2.x b111361)
    Cloning b1113613d8fc2ec5762e073306d99906bb24eb0e
    b1113613d8fc2ec5762e073306d99906bb24eb0e is gone (history was rewritten?)

Installation failed, reverting ./composer.json to its original content.


  [RuntimeException]
  Failed to execute git checkout 'b1113613d8fc2ec5762e073306d99906bb24eb0e' -- && git reset --hard 'b1113613d8fc2ec57
  62e073306d99906bb24eb0e' --
  fatal: reference is not a tree: b1113613d8fc2ec5762e073306d99906bb24eb0e

This happened with both commerce and xero modules. Drupal packagist displays the correct hash for the version (dev-8.x-2.x and dev-8.x-1.x respectively), but maybe it's cached?

Job definition array for composer step:

            [composer] => Array
                (
                    [0] => install --prefer-dist --working-dir 
                    [1] => config repositories.drupal composer https://packagist.drupal-composer.org --working-dir 
                    [2] => require mile23/drupal-merge-plugin --working-dir 
                    [3] => require drupal/commerce:dev-8.x-2.x --working-dir 
                )

Also the use of the merge plugin directly invalidates the use of setup/checkout for additional repositories.

Should drupalci not checkout if it's going to use composer require the modules via drupal packagist?

There's also no way to turn this off for contrib because there's nothing to check if something has composer dependencies during preprocess of the job definition. This is fine if drupalci just uses composer to get the module as long as there is no inconsistency with git.drupal.org and composer.

Okay, here's a working patch with the merge plugin.

I thought I would like to use the require command, but since we're already checking out the modules from git, then just using update command, which @Mile23 mentioned in his above comment.

I don't run into any git hash issues with this.

I have not tested this with DCI_Patch/DCI_Fetch yet.

Edit: Patch/Fetch works.

Tested with payment, commerce, and xero modules on 8.1.x. I haven't tried 8.2.x but should be similar.

drupalci config for payment:

DCI_DBVersion=pgsql-9.4
DCI_PHPVersion=5.6
DCI_TestItem=directory:modules/payment
DCI_CoreBranch=8.1.x
DCI_JobType=simpletest
DCI_AdditionalRepositories=git,git://git.drupal.org/project/payment.git,8.x-2.x,modules/payment,1;git,git://git.drupal.org/project/currency.git,8.x-3.x,modules/currency;git,git://git.drupal.org/project/plugin.git,8.x-2.x,modules/plugin
DCI_TestGroups=Payment
DCI_ComposerInstall=True
DCI_RunScript=/var/www/html/core/scripts/run-tests.sh
DCI_DBUrl=pgsql://drupaltestbot:drupaltestbotpw@host/drupaltestbot
DCI_Concurrency=1
DCI_CoreRepository=git://git.drupal.org/project/drupal.git
DCI_GitCheckoutDepth=1
DCI_RunOptions=sqlite /var/www/html/results/simpletest.sqlite

Module Fails (fatal errors causing test runner to return non-zero error code):
- Drupal\Tests\payment\Unit\Plugin\Payment\MethodConfiguration
- Drupal\Tests\payment_reference\Unit\Element\PaymentReferenceTest
- Drupal\Tests\payment\Unit\Entity\Payment\PaymentStatusFormTest

drupalci config for commerce:

DCI_DBVersion=pgsql-9.4
DCI_PHPVersion=5.6
DCI_TestItem=directory:modules/commerce
DCI_CoreBranch=8.1.x
DCI_JobType=simpletest
DCI_AdditionalRepositories=git,git://git.drupal.org/project/commerce.git,8.x-2.x,modules/commerce,1;git,git://git.drupal.org/project/entity.git,8.x-1.x,modules/entity,1
DCI_TestGroups=commerce
DCI_ComposerInstall=True
DCI_RunScript=/var/www/html/core/scripts/run-tests.sh
DCI_DBUrl=pgsql://drupaltestbot:drupaltestbotpw@host/drupaltestbot
DCI_Concurrency=1
DCI_CoreRepository=git://git.drupal.org/project/drupal.git
DCI_GitCheckoutDepth=1
DCI_RunOptions=sqlite /var/www/html/results/simpletest.sqlite
# Patch testing:
# DCI_Fetch=https://www.drupal.org/files/issues/2691591-3.patch,modules/commerce
# DCI_Patch=2691591-3.patch,modules/commerce

Module Fails (however all phpunit unit tests pass):
- Throws a lot of exception in tests ("Found database prefix 'simpletestN' for test ID Y")
- Drupal\Tests\commerce_order\Kernel\Entity\OrderTest (fatal)
- Drupal\Tests\commerce_product\Kernel\ProductAttributeFieldManagerTest (fatal)

drupalci config for xero:

DCI_DBVersion=pgsql-9.4
DCI_PHPVersion=5.6
DCI_TestItem=directory:modules/xero
DCI_CoreBranch=8.1.x
DCI_JobType=simpletest
DCI_AdditionalRepositories=git,git://git.drupal.org/project/xero.git,8.x-1.x,modules/xero,1
DCI_TestGroups=Xero
DCI_ComposerInstall=True
DCI_RunScript=/var/www/html/core/scripts/run-tests.sh
DCI_DBUrl=pgsql://drupaltestbot:drupaltestbotpw@host/drupaltestbot
DCI_Concurrency=1
DCI_CoreRepository=git://git.drupal.org/project/drupal.git
DCI_GitCheckoutDepth=1
DCI_RunOptions=sqlite /var/www/html/results/simpletest.sqlite

Module Fails: (has some fatals related to phpunit mocks)

I made a new branch for this issue, with only the changes in #18. (I had to use --reject since the patch wouldn't apply otherwise)

Performed vagrant up process.

I set up a config file per the Xero example above, except with php 5.5 and mysql 5.5 just to save on download time.

Initially, the testbot had trouble finding composer, which should have been solved by #2687303: DCI_ComposerInstall fails under vagrant but since the patch in #18 deletes that file, the change wasn't preserved.

After changing Composer::buildComposerCommand to return "./bin/composer $data $workingdir"; it all happened smoothly.

Of course it took approximately three years to finish the composer phase, because we have to do all of the following:

• Clone Drupal
• Run composer install for Drupal which is pretty quick.
• Require drupal-merge-plugin, which has to then check all the dependencies.
• Run composer update using the merge plugin, which then checks all the dependencies again.

Obviously this would be quicker if the merge plugin were a dependency of core. :-) But that's out of scope here, and we should determine its suitability here first.

There's some testing of the composer process over here: #2690439: Test the behavior of Composer Buildstep

We should have a functional test here, too, using DrupalCIFunctionalTestBase.

And of course after all that here's the output of the tests:

$ ./bin/phpunit 
PHPUnit 4.8.23 by Sebastian Bergmann and contributors.

Cloning into '/var/lib/drupalci/web/simpletest_1458928651/modules/drupalci_d8_module_no_tests'...
remote: Counting objects: 11, done.
remote: Compressing objects: 100% (10/10), done.
remote: Total 11 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (11/11), done.
Checking connectivity... done.
PHP Fatal error:  Class 'DrupalCI\Plugin\BuildSteps\generic\SetupBase' not found in /home/vagrant/drupalci_testbot/src/DrupalCI/Plugin/BuildSteps/generic/Composer.php on line 19

There's now a TESTING.md file which tells you how to run the tests. :-)

So from #21, it sounds like we are adding a significant amount of overhead to a test run, in order to support the case where a contrib module under test is dependent on non-drupal composer dependencies that aren't already required by drupal core.

How often is this likely to be the case? How much of the above is needed for basic contrib testing, versus exceptions?

@jthorson: Based on what I've seen so far in the D8 ecosystem, quite often. It's already a blocker to most of the Drupal Media's ecosystem of modules for testing, which have to use TravisCI. I would love to see those projects come back to Drupal.org entirely.

@davereid +1

Im using travis for this reason, and because of Drupal dependencies.

DrupalCI seems to be testing my contrib modules with the latest release of a dependency, for example when testing 8.x-1.x of a module I'd want 8.x-1.x of the dedependencies, not 8.x-1.0-beta1. Then for core I want 8.0.x, not 8.2.x

Okay ... just trying to understand the requirements, before jumping in with implementation suggestions.

Re: #25 The good news is that the test runner supports running whatever combination of branches it's told to run ... so the current limitation resides solely within the drupal.org integration (and, for example, a user interface that would allow you to specify the desired combinations).

Next question ... can someone explain to me the motivation behind the drupal-merge-plugin, as opposed to simply running "composer install" in each module's checkout directory?

Today, the DrupalCI composer plugin assumes that it should always run from the base test working directory; but if we broke this assumption, it would be relatively trivial to add a "composer install sites/all/modules/MYMODULE" after all of the repository checkouts have been completed.

@jthorson
The entire Commerce ecosystem will need this. We're on Travis currently because of it.

@bojanz
I understand you need composer runs ... but I'm trying to understand what specific need the drupal-merge-plugin meets that running 'composer install' in each module's directory does not; in case there might be something we can do within the DrupalCI architecture to simplify the implementation.

@jthorson
I think if composer install is run for each module separately, you risk that the same library is cloned again, which could lead to "Cannot redeclare class" errors.

Example if modules "qux" and "baz" both depend on the same library:
Fatal error: Cannot redeclare class Foo\Bar in /drupal/modules/qux/vendor/Foo/Bar.php on line 5, already defined in /drupal/modules/baz/vendor/Foo/Bar.php

Running composer install in each module wouldn't work, it would produce multiple vendor directories which wouldn't get picked up by autoloader.

Great ... thanks for the explanation.

Based on that, the approach in #18 is the right way to go. I'll take a look at verifying the patch and updating the test, but after that, I'd say this is ready to go.

Hmmm ... Composer.php should extend PHPUnit, as getting rid of the do-nothing exec() wrapper eliminates the need for SetupBase. That might even be enough to fix the test.

And wow ... I *really* don't like how this approach extends the time it takes to run a simple one-group local test by about 400%!!!!

Also ... DCI_UseLocalCodebase results in $setup['checkout'] = ['protocol' => 'local', 'source_dir' => 'source_dir'], so this is getting added/executed even when there are not additional repositories.

Edit: in other words, we can't simply count $setup['checkout'] elements.

Did some tweaking on the branch:

1. Added priority arguments to SimpleTest job:
   • Ensure that DCI_UseLocalCodebase is processed before DCI_AdditionalRepositories, as it replaces the complete 'checkout' array when processed,
   • Ensure that DCI_AdditionalRepositories is processed before DCI_ComposerInstall, as the latter depends on the output of the former's definition preprocessor.
2. Adjusted the ComposerInstall plugin to execute the original composer install line for core-only tests, and not process it if we are going to be processing the composer update later anyway.
3. Adjusted the Composer plugin to extend PluginBase instead of SetupBase (and removed the call to the SetupBase's do-nothing exec() wrapper)
4. Added a check to ensure $setup['checkout'][0] is an array, so that use of DCI_UseLocalCodebase alone doesn't trigger the additional composer logic

Subsequent local test runs are much faster than my initial run, thanks to Composer's cache; and between this and removing the redundant composer install run makes the overhead less of an issue.

Local run now getting similar failures for Payment as in #19.

All existing (i.e. already merged) DrupalCI tests passing.

DrupalCI seems to be testing my contrib modules with the latest release of a dependency, for example when testing 8.x-1.x of a module I'd want 8.x-1.x of the dedependencies, not 8.x-1.0-beta1. Then for core I want 8.0.x, not 8.2.x

Unfortunately the root drupal/drupal sets a minimum stability of dev with prefer-stable. Your module can specify stability of stable but drupal/drupal's will override it.

As far which core branch you get, that's up to d.o's CI launcher (PIFT) to specify. There is currently no way to specify it; you're stuck with the latest core dev branch (currently 8.2.x) until you re-run the test manually. There are a few issues about it. Start here: #2651208: More-flexible, while reasonable, configured tests

And wow ... I *really* don't like how this approach extends the time it takes to run a simple one-group local test by about 400%!!!!

No kidding. If it were in core it would be one update instead of two which is still a bunch of time. We can't really use a composer.lock or anything because we don't know what's been patched.

I just confirmed that the tests pass now. We really should have a test similar to ContribNoTestsTest except with a composer.json file specifying a dependency.

We really should have a test similar to ContribNoTestsTest except with a composer.json file specifying a dependency.

Well, I've manually run a combination of tests, including with/without DCI_UseLocalCodebase, with/without DCI_AdditionalRepositories, and with DCI_ComposerInstall set to both TRUE and FALSE ... I can't really imagine this introducing a regression or breaking something in a 'subtle' way. Either the extra composer commands run or they don't, and the unit tests in #2690677: Generate test coverage for definition and variable preprocessors could be tweaked to verify that the commands get added to the definition file properly, once they get merged in. Does a full end-to-end functional test really add enough value to delay this further?

Does a full end-to-end functional test really add enough value to delay this further?

We're undertaking this exercise (making a CI system) in the first place so we can do two things:

1) Automatically prove that the feature we're introducing works.

2) Automatically prove that future changes don't break this feature.

That can only happen if there are tests to run.

Added another contrib project called drupalci_d8_module_composer which is located in a sandbox here: https://www.drupal.org/sandbox/mile23/2695805

It uses composer to require the crell/api-problem package. Once we get d.o-based library projects we should swap it out for one of our own so we can save other people's bandwidth and not turn Crell's use stats into a lie. :-) The module has a single PHPUnit-based test which just instantiates an ApiProblem object and then checks if it's NULL.

Added a test to drupalci called DrupalCI\Tests\Application\ContribComposerTest. This is a near-duplicate of ContribNoTestsTest, but instead looking to make sure the module's phpunit test ran.

Thank you for taking the time to explain why people write tests. But that completely ignores the point of my question. (Not to mention coming across as more than a little patronizing ... I did maintain the testing infrastructure for five years, I'm not a complete noob here!)

#2690677: Generate test coverage for definition and variable preprocessors provides unit test coverage for each of the definition and variable preprocessor plugins. A slight tweak of the existing ComposerInstall test gives us test coverage of this feature. Adding an end-to-end functional test run adds significant overhead to the test suite, for what I'm arguing is marginal value at best, over and above the unit tests. Once again, these are not things that break in subtle ways ... we should not need to add a brand new 2 minute end-to-end test run to the suite for each new feature - one or two catch-all functional tests should be more than enough.

We are seriously overcompensating with these repeated demands for end-to-end functional test runs ... heck, I even had a two-line bugfix reverted, despite it being RTBC'd six months ago, stating that we needed functional test coverage before it would be accepted - when the consequence of that bugfix breaking is that the PHP binary is not found. Do we really need to add another 2 minutes to the test suite to make sure that we substitute the correct php binary during a test run? No! The unit test takes care of that coverage for us ... and yet those unit tests sit unreviewed for over a week while we write functional tests for the same features.

I know that I've been labelled as 'anti-testing' (which is the strawman that keeps getting thrown back at me any time I try to raise an objection) ... and that's the vibe I got from your response. But isolating my question and taking it out of the context of the rest of the post in order to reinforce that strawman really isn't fair.

When you say: "Well, I've manually run a combination of tests, including with/without DCI_UseLocalCodebase, with/without DCI_AdditionalRepositories, and with DCI_ComposerInstall set to both TRUE and FALSE ... I can't really imagine this introducing a regression or breaking something in a 'subtle' way.""

...you are saying that you performed an end-to-end test which proved to you that the feature works.

Therefore we should automate the test that proved this to you.

In this issue, we are talking about proving that drupalci adds composer dependencies for contrib, which is a new behavior. If a test in another issue branch dealing with a codebase that doesn't provide that feature is capable of proving the specific new behavior from this issue, you're going to have to show me how.

If you have a better test than the one I provided then please offer it here. Please make it specific to the feature we're working on here.

So there are a couple things here:

1. Does mile23/drupal-merge-plugin really work, and is it appropriate?
2. Do our modifications to drupalci work or break or what? Seems to work.
3. Do we need tests? I say yes, jthorson says no.

Anyone care to jump in?

Any issues/PRs to drupal-merge-plugin welcome. We can move it to d.o infrastructure or to the drupal-ci github group as desired.

Do we need tests? I say yes, jthorson says no.

I REALLY wish people would stop misrepresenting me like that.

The preprocessor unit tests can ensure that the composer lines get inserted into the definition file at the appropriate place. A unit test for the build step plugin would ensure that, when they are inserted, they actually get executed. My stance is that adding an additional 'functional' test for every plugin is redundant, and adds significant overhead to the testing suite, for marginal added value, over and above the unit testing. It's not the DrupalCI Test Suite's responsibility to validate that when you run those three composer commands, that the drupal site is successfully installed ... the DrupalCI test suite should only need to verify that the appropriate commands get added to a test run in the appropriate scenarios.

Arguing that we do not need dedicated functional test runs for every single new feature that is being added is NOT the same as "We do not need testing".

A unit test for the build step plugin would ensure that, when they are inserted, they actually get executed.

First you've mentioned it.

Is there one?

Nope.

I think this is major since basically no contrib modules relying on Composer to load dependent PHP libraries can be tested on DrupalCI. See for example #2750351-6: Add a 'font' FileMetadataPlugin to access TTF/OTF/WOFF font information and #2698237: Make branch test pass on DrupalCI

Anyone interested in this issue can check out the 2597778-composer-for-contrib branch and run the testbot locally: https://www.drupal.org/node/2487065

Report back please. :-)

Also we have to figure out what tests to include, since that seems a bit contentious. @jthorson mentions adding some unit tests, which would be great to have. I think a functional test is important since there are so many moving parts here.

I just did some refreshing at https://github.com/paul-m/drupal-merge-plugin to add travisCI and to update the readme a little. If anyone could review that project it would be of great help.

The main down-side of this issue is that it will take quite a bit of time to assemble the dependencies. Any extension being tested that has Composer-based dependencies will trigger this process, which adds a considerable amount of time to the test. However, there's not really any way around that, and any strategy there would have to be about mitigation. Like, maybe having all testbot machines share the same Composer cache directory or something.

Can someone give us a status update here? Do we interpret it correctly that not a single contrib project is able to run tests if they depend on another composer package?

How is this not critical? This totally prevents the Drupal ecosystem from "getting off the island".

Can someone give us a status update here?

The patch needs to be redone, but is probably blocked on drupalci architecture changes.

The general approach needs to be

1. Change it so that modules are checked out and patched into temporary directory.
2. Add composer vcs repos for the module that was checked out.
3. Add composer repo for packages.drupal.org.
4. Composer require the module.

Do we interpret it correctly that not a single contrib project is able to run tests if they depend on another composer package?

This is true only for drupalci. The test runner itself does not care, and I run and develop tests for my modules with composer dependencies. I do it with travis until drupal.org is ready to support it.

Here is what I finally figured out to support both push builds and pull request builds: http://cgit.drupalcode.org/xero/tree/.travis.yml. It is essentially the same thing as what needs to be done above.

This totally prevents the Drupal ecosystem from "getting off the island".

Technically contrib. modules with composer dependencies are more off the island now than anything else. :D

but is probably blocked on drupalci architecture changes

Just yesterday I merged in the last of the massive refactoring of drupalci, so the architecture changes are there, meaning it'll be much simpler to add this functionality to drupalci. Next step is to modify the AWS AMI to support some of the changes (php7, mostly). Then we can deploy it to production, and get drupalci back into a state where we can do continuous delivery.

@mradcliffe: Im trying to recall why we established that procedure at drupalcon Dublin w/webflo - shouldnt we be able to

1. Add composer repo for packages.drupal.org.
2. Composer require the module.
3. Apply any patches
4. Composer install again if patch affects composer.json

I could be misremembering if composer would pick up the delta in a patched composer.json in a contrib module or not.

Then we can deploy it to production, and get drupalci back into a state where we can do continuous delivery.

Woot!

I could be misremembering if composer would pick up the delta in a patched composer.json in a contrib module or not.

It won't, unless we use drupal-merge-plugin from #51, or the technique in #55.

+1 on #55, due to speed.

Just wondering if the huge set of commits in #59 have caused #2837015: CI problem with permission when testing 7.x patches. All 7.x patch testing has been failing with 'CI error' since 16 Dec.

No, all of @Mixologic's work on this is happening in the dev branch, not the production branch.

Well, the 2597778-composer-patched-dependencies branch. :-) Up at the top under 'related issues,' you can see the issue branch name.

OK. Thanks for explaining about the branches. I should have spotted that.

Hello hi and howdy.

DrupalCI's refactor was deployed to production today, and it included the ability to construct the codebase using composer.

The workflow is similar to #55:

1. clone core
2. composer install core
3. add packages.drupal.org as a composer repo.
4. composer require drupal/<project_under_test>
5. read /vendor/composer/installed.json and find any require-dev dependencies of the <project_under_test>, and composer require those as well (i.e make sure that test-dependencies from info-yml are part of the test build)
6. apply any patches to the <project_under_test>
7. if composer.json is one of the files modified,
8. move modules/contrib/<project_under_test> to an ancillary directory
9. add ancillary directory as another composer path repo
10. composer require drupal/<project_under_test> -> this will get the new dependencies 

Run the tests.

So, external composer dependencies will now be pulled in as part of testing. Hooray.

In documenting these steps I realize that I need to redo step 5 after step 10 in case any require-dev dependencies are added to the patch. (issue here: #2837916: Ensure that a patch that adds require-dev works when building the codebase.)

Otherwise yay. have funz and happy festivus.

See it in action: https://www.drupal.org/pift-ci-job/556243

Thank you, great work!

This fixed #2698237: Make branch test pass on DrupalCI; now the ua-parser module automated tests work fine - the ua-parser/uap-php dependency is installed properly before runnig the test.

However, I have a different issue with #2750351: Add a 'font' FileMetadataPlugin to access TTF/OTF/WOFF font information. This patch introduces a sub-module (file_mdm_font) within the main module (file_mdm). The submodule has a composer.json of its own which introduces a dependency on phenx/php-font-lib. The idea is to have it as a dependency of the sub-module because only the submodule needs it - not the main module.
The sub-module dependency is not installed, though.
See test https://dispatcher.drupalci.org/job/default/283084/console

Sorry to reopen this, please let me know if a separate issue is needed or if alternative solutions should apply.

@mondrake
You can't use Composer that way. There is one composer.json for the module and all of its submodules.

#70 ah OK then, will move submodule requirements on the main module composer.json.

Thanks @bojanz.

@bojanz @mondrake - The setup that mondrake describes is actually the one that we would want to encourage - it is pretty much the monorepo/manyrepos pattern that Fabien Potencer has described in many talks, and would help us bridge the gap between the world of 'projects' where we have multiple modules per project, and the world of composer.

This is also the setup we use in drupal core - each component has its own composer.json. The only thing we're lacking currently is the subtree splits to offer up the components as separate repositories.

Right now we need to be able to support *both* ways of doing it as detailed here: #2754747: Document root composer.json details.

As far as testing is concerned, what this means is that we need to composer require every module that comes with a project.

Strange as this sounds, one way this could currently be accomplished is by adding your submodules as a test_dependency/require-dev to your info.yml/composer.json

Composer would then get your submodules dependencies for testing purposes. Unless, of course, even *those* have dev-dependencies, in which case those, too, should be specified at the main project composer.json. (This makes it *very* complicated for somebody who has a project name that doesnt match any of their module names, like http://drupal.org/project/mutual_credit)

Another example is https://www.drupal.org/project/smart_ip

Does this work for D7 as well? Do we need a dependency on composer manager?

nyariv : we are using composer to require the modules for d7, but I havent yet tested a d7 module with an external dependency.

Im also not sure if composer manager works in the simpletest environment (when modules are enabled for tests) if it finds modules and their dependencies.

Basically, I personally havent used composer manager in d7 except on d.o. for the symfony yaml part.

So if we also want d7 builds to include external dependency resolution, you could probably help out with #2348215: Add support for Composer Manager in Drupal 7 test runs and let me know what the build process needs to be to get a module ready to test.

So theoretically, I could include the autoload.php in the root directory in my tests, assuming my d7 module has a composer.json, without needing composer manager. Is this correct?

The above method indeed works, I have tested this on Mobile Number.

I added the following code above the test class declaration:
include DRUPAL_ROOT . '/vendor/autoload.php';

All tests passed with no error. Thanks Mixologic.

This is failing for the JSON API module, in an issue where we're adding a new PHP library to validate JSON API responses.

See #2840677-12: Validate responses against the JSON API specification. If you look at DrupalCI's log, you'll see the new library (justinrainbow/json-schema) is not being installed.

According to the comment in #66, which describes the algorithm used, patches adding new libraries should also work:

6. apply any patches to the <project_under_test>
7. if composer.json is one of the files modified,
8. move modules/contrib/<project_under_test> to an ancillary directory
9. add ancillary directory as another composer path repo
10. composer require drupal/<project_under_test> -> this will get the new dependencies 

So… what is that patch doing wrong? Or is it perhaps broken? (The see it in action link in #68 only shows it for a branch that already has a composer.json.)

Perhaps we've found an edge case, because this patch is not modifying a composer.json file, it's adding one?

Yes, this does not seem to be supported yet. :(

The module needs to be checked out into a temporary directory, patch applied, a composer repository added, and then the composer require command can be run.

Probably deserves a new issue.

Well, according to #66, this is supposed to be supported. So, reopening.

Perhaps we've found an edge case, because this patch is not modifying a composer.json file, it's adding one?

@Wim Leers: I think your theory was right. List of modified files is a result of git diff --name-only

http://cgit.drupalcode.org/drupalci_testbot/tree/src/DrupalCI/Build/Code...

Shoot. I noticed that git diff -name-only bug on another branch, and have fixed that, but its not yet deployed: http://cgit.drupalcode.org/drupalci_testbot/tree/src/DrupalCI/Build/Code...

Branch 1266444-phpcs-coder from #1266444: DrupalCI should return failed for poor code style - php is merged into dev.

FYI: I had mentioned in #66 that adding a require-dev in a patch didn't work. I fixed that today: #2842160: When composer.json is patched, we should detect new require-dev dependencies

But it want using require-dev! But I think it should, so I'm glad to hear that's now also supported :)