Drupal\Tests\ComposerIntegrationTest assumes there is a composer.json in DRUPAL_ROOT

Here's an initial patch, it just does a simple file_exists, then fails if the file is missing, unless it's ./composer.json.

I was thinking we should return some sort of message if ./composer.json is missing, but not fail the test, not sure how useful that would be though.

I am not sure how to do this ... maybe someone else could comment I would be really grateful

If the thrust of this issue is expanded to :-

"Make sure the file exists AND it has no errors in it" then we would solve the testing required for this issue #2545430: Remove "UnexpectedValueException" from composer.json

currently the only way I know to add this is through the

"drush composer diagnose" command

+++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
@@ -60,11 +60,17 @@ protected function getPaths() {
+      if (file_exists($file)) {

I think we should not introduce the file_exists for all composer.json because this fails silent if we remove one of files accidentally. I think it makes more sense to add a special case for root composer.json because this files may does not exists in a project managed with composer.

+++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
@@ -60,15 +59,27 @@ protected function getPaths() {
+    if (file_exists($this->root . '/composer.json')) {
+      $this->validateComposerJson($this->root . '/composer.json');
+    }

It would be great to document this file_exists() call. The reasons given by webflo make total sense.

Awesome, thanks!

I'm not entirely convinced by this change since this test is for testing core so that file should be there - there are many tests that would fail if tested not on a clean checkout of drupal core. Also there is an issue suggesting making this the main composer json.

Thanks correct. Lets focus our efforts on #2380389: Use a single vendor directory in the root. We have to get it in before RC1.

Agree with @davidwbarratt.

Queued the patch in #6 for retesting. Think it's still valid.

I think this is wrong thinking - tests are designed to run against the checkout - if you move things around they break - that is how it is supposed to be. There is no response to my point in #8.

Since ./composer.json is an example file (like example.gitignore), then ...

The root composer.json file isn't an example file.

If you need to run all the unit tests against an installation where you've moved the composer.json file around, then you can use ./vendor/bin/phpunit -c core --exclude-group Composer We might also make a feature request for run-tests.sh to be able to exclude groups.

Alternately, if there were some way to determine whether the test were being run to test core or test something else, then it might be fair to mark the test as skipped under that circumstance. But that seems a long way to go for an edge case. The file_exists() call negates part of the purpose of the test otherwise.

Agreed with the point in #14... there's nothing to say Drupal couldn't be included inside another project, with a top-level composer.json.

Regardless, though, I've come to include a composer.json file in the webroot with a simple contents of {} to appease this test, but also composer_manager, which last time I checked also expected to find this file as valid JSON. Just in case you're looking for a quick, non-patch fix.

I thought: Hey, let's add the collected wisdom of this issue to the documentation of ComposerIntegrationTest.

Then I saw getPaths() and thought: That should be a data provider.

Lo the yak was shaved, and I ended up adding a getRoot() static method to UnitTestCase.

Presented here: The full code-changes patch, and also the docs-only patch.

+++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
@@ -42,30 +52,35 @@ protected function getErrorMessages() {
+    // Data provider functions are run before setUp(), so we have to determine
+    // DRUPAL_ROOT here.
+    $root = self::getRoot();

Even is the provider is executed before setup() we still don't need a static method, IMHO

Even is the provider is executed before setup() we still don't need a static method

Well, then it won't be useful for setupBeforeClass() either.

But we can just do the docs change in patch 2545344_17-docs-only.patch if that's intractable or out of scope.

Since this was last working, Composer use has basically become required for installing Drupal core from source, and there's also a new test method to compute the hash of the lock file. See http://cgit.drupalcode.org/drupal/commit/core/tests/Drupal/Tests/Compose...

So, including an empty composer.json file is no longer enough to stave off this failure.

I think it would be worthwhile at this point to see if a core maintainer thinks it's worth supporting non-out-of-the-box installation methods that cause this to be an issue, or to simply document this and require the dev to exclude this test suite?

Here is a general question. Why does one run the core tests in their custom projects? It is not the job of each individual site to prove that core works. Maybe though I misunderstand the question. Beside of that the patch here certainly improves the quality of the patch though.

@dawehner: If you run tests using the core tools, the core tests will be discovered and possibly run.

The solution to this problem is to mark the test as incomplete if the file is not present, probably in setUp().

I still think its not the responsibility for you to run those tests and its not the responsibility for Drupal to support arbitrary paths of stuff in its test.
Its also not that our tests are smoke tests for your environment.

Just an idea: We could maybe separate unit from unit-core in our phpunit.xml file and so exclude all those tests easily.

I run all core unit tests on https://github.com/drupal-composer/drupal-project. We caught a few bugs around autoloading that way, because the project changes the location of the vendor folder.

Looks like drupal-project does this in travis-ci:

- ./../vendor/bin/phpunit -c core --testsuite unit --exclude-group Composer

@group Composer only applies to this test.

So that's either a workaround for a problem in core or it's a solution to the policy in core. Which do we think it is?

Why do not use drupal-finder package?

It's already used in the drush and drupal console for the searching of the composer and drupal root, so seems like it's a stable package which works fine. I don't see any reasons why we can't use it here.

There is a patch for ComposerIntegrationTest. The idea was taken from #17 but code was rewritten to use DrupalFinder class.

Just realized that dirname() accept second parameter only in php7

Patch is updated.

I still think its not the responsibility for you to run those tests and its not the responsibility for Drupal to support arbitrary paths of stuff in its test.

I agree with this, it's not for every site to run all tests.

I run all core unit tests on https://github.com/drupal-composer/drupal-project. We caught a few bugs around autoloading that way, because the project changes the location of the vendor folder.

I'm pretty sure that this project is not just another project, excluding this test-group is a good idea for that.

The docs-only patch in #17 is a good idea, otherwise I think we should just close this issue.

I'd +1 a docs-only change... if you are sophisticated enough to get WTF'd by this you will be able to do some investigating and the proposed docs patch is clear enough to give you some sensible options, while not changing anything functionally on core's end.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.