Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Broken tests in master: Could not scan for classes inside "tests/" #20032

Closed
linaori opened this issue Sep 23, 2016 · 37 comments · Fixed by #20317
Closed

Broken tests in master: Could not scan for classes inside "tests/" #20032

linaori opened this issue Sep 23, 2016 · 37 comments · Fixed by #20317

Comments

@linaori
Copy link
Contributor

linaori commented Sep 23, 2016

It seems like some commit broke at least the master. If I run ./phpunit it crashes after installing the packages with:


  [RuntimeException]                                                                          
  Could not scan for classes inside "tests/" which does not appear to be a file nor a folder  

I did a bit of digging and the only reference I found was a classmap option in composer.json from the zip file in the .phpunit directory. Sadly I was unable to fix it myself as it keeps overwriting everything.

ping @nicolas-grekas

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 23, 2016

where does this tests/ string come from?

@nicolas-grekas
Copy link
Member

Any backtrace?

@linaori
Copy link
Contributor Author

linaori commented Sep 23, 2016

I can't summon a back-trace, I think it's invoked by a sub-process of the phpunit bash file that runs an include. I can't find tests/ other than in a zip file where it's defined in the class-map options.

@stof
Copy link
Member

stof commented Sep 23, 2016

@iltar do you have a phpunit.xml file locally, replacing the phpunit.xml.dist shipped in the repo ?

@linaori
Copy link
Contributor Author

linaori commented Sep 23, 2016

No, I have everything stock, I was reproducing this issue for @stampycode

@linaori
Copy link
Contributor Author

linaori commented Sep 23, 2016

A bit more info:
image

@alcohol
Copy link
Contributor

alcohol commented Sep 23, 2016

I can get the test suite to run (though with a lot of failures) by modifying the following binary:

vendor/symfony/phpunit-bridge/bin/simple-phpunit

diff --git i/bin/simple-phpunit w/bin/simple-phpunit
index 14a9d1c..a8c1581 100755
--- i/bin/simple-phpunit
+++ w/bin/simple-phpunit
@@ -53,8 +53,8 @@ if (!file_exists("$PHPUNIT_DIR/phpunit-$PHPUNIT_VERSION/phpunit") || md5_file(__
     if (5.1 <= $PHPUNIT_VERSION && $PHPUNIT_VERSION < 5.4) {
         passthru("$COMPOSER require --no-update phpunit/phpunit-mock-objects \"~3.1.0\"");
     }
-    passthru("$COMPOSER require --dev --no-update symfony/phpunit-bridge \">=3.2@dev\"");
-    passthru("$COMPOSER install --prefer-dist --no-progress --ansi", $exit);
+    passthru("$COMPOSER require --no-update symfony/phpunit-bridge \">=3.2@dev\"");
+    passthru("$COMPOSER install --no-dev --prefer-dist --no-progress --ansi", $exit);
     if ($exit) {
         exit($exit);
     }

The zipfile downloaded by this binary does not contain the tests/ directory since the .gitattributes exclude it from github zipballs. Yet the composer.json contains an autoload-dev definition which references this directory, and the binary runs a composer install (including dev) which results in a failing classmap generation.

@stof
Copy link
Member

stof commented Sep 23, 2016

autoload-dev is taken into account only for the root project (which is not downloaded by composer, except when using create-project), so there is no reason for it to make any difference here unless a recent version of composer introduced a bug. what is your composer version ?

@nicolas-grekas
Copy link
Member

Did you try deleting the .phpunit/ folder then try again?

@alcohol
Copy link
Contributor

alcohol commented Sep 23, 2016

@stof you are wrong. The composer install is run inside the extracted phpunit directory (making phpunit the root project at that moment), and directly affects the phpunit dependencies, so autoload-dev definitely is taken into account.

@linaori
Copy link
Contributor Author

linaori commented Sep 23, 2016

My version: Composer version 1.2.1 2016-09-12 11:27:19

@alcohol
Copy link
Contributor

alcohol commented Sep 23, 2016

See https://github.com/symfony/phpunit-bridge/blob/master/bin/simple-phpunit#L46-L57 for relevant code that causes this issue.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 23, 2016

So, composer is throwing the exception? Can you add -vvv to the corresponding composer call?

@alcohol
Copy link
Contributor

alcohol commented Sep 23, 2016

I already explained why. See this comment.

Symfony attempts to install the dependencies for the phpunit version it downloaded (using composer). However, the github zipball does not contain the tests directory because of the .gitattributes configuration. The install it tries though includes the dev dependencies, and thus also triggers the parsing of the autoload-dev configuration, which references the non-existant tests directory.

@alcohol
Copy link
Contributor

alcohol commented Sep 23, 2016

Probably relevant: sebastianbergmann/phpunit#2300

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 23, 2016

certainly relevant, that's the trigger of all of this :)
can you submit a PR then?

@linaori
Copy link
Contributor Author

linaori commented Sep 23, 2016

My guess is a simply mkdir for /tests would be the easiest

@alcohol
Copy link
Contributor

alcohol commented Sep 23, 2016

Sure. On what branch should this be applied?

@nicolas-grekas
Copy link
Member

the lowest impacted :)
but shouldn't composer deal with this more gracefully?

@alcohol
Copy link
Contributor

alcohol commented Sep 23, 2016

I suppose that is debatable. Composer has always failed on this. This is not new. We could attempt to handle it more gracefully, but the problem is that a lot of users rarely read the warnings we output. And as this could effectively lead to a completely broken state of your environment, I feel a warning is inadequate.

@linaori
Copy link
Contributor Author

linaori commented Sep 23, 2016

I think the proper fix would be that composer doesn't crash on this during the construction of the class-map autoloading and give an understandable error. Composer could give a neat warning in yellow though (just like the out-of-date) so that you are at least hinted of it.

@Seldaek
Copy link
Member

Seldaek commented Sep 23, 2016

Going to install deps and generally mess around inside the vendor dir definitely voids the composer warranty as far as I am concerned. And besides yes as @alcohol mentioned above, people don't read warnings and then complain, that's why we fail hard on this because it is a clear misconfiguration of the package. Either make sure you create the tests dir, or install with --no-dev so that the autoload-dev is ignored, which really would be the correct option IMO as you are trying to run phpunit as far as I understand, and not develop it.

@alcohol
Copy link
Contributor

alcohol commented Sep 23, 2016

The latter was my solution also (install --no-dev), as displayed in the patch I included earlier in this thread.

@alcohol
Copy link
Contributor

alcohol commented Sep 23, 2016

Only applies to the master branch, none of the previous branches had this binary yet.

@stampycode
Copy link

This fix doesn't work for me, do I need to clear anything after I've pulled in this change?

I'm running php ./phpunit symfony

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 23, 2016

@stampycode try deleting the .phpunit/ folder before running the command?

@stampycode
Copy link

@nicolas-grekas ... no dice.

@alcohol
Copy link
Contributor

alcohol commented Sep 23, 2016

That's because it hasn't been merged yet.

@nicolas-grekas
Copy link
Member

OK, that's because the simple-phpunit that is used is the one in vendor/symfony/phpunit-bridge/bin/simple-phpunit, not in src/...

@stampycode
Copy link

@alcohol I applied the fix manually.

@alcohol
Copy link
Contributor

alcohol commented Sep 23, 2016

Yes but to what? ;-)

@stampycode
Copy link

stampycode commented Sep 23, 2016

@alcohol - lol, as in #20034 - I did exactly what it said, to the file described in the PR. I'm not new to this game ;)

@alcohol
Copy link
Contributor

alcohol commented Sep 23, 2016

To which file? Did you read the comment from @nicolas-grekas above?

@stampycode
Copy link

Yeah I did - but as far as I'm concerned, I'm running the unit tests as described in the documentation - if that doesn't work, then that means either the documentation is wrong, or the tests are still broken...

And yes, I've deleted the .phpunit dir between runs.

@stampycode
Copy link

Urgh. Ok it just started working... fml.

@fabpot fabpot closed this as completed in f418cb8 Sep 23, 2016
fabpot added a commit that referenced this issue Sep 23, 2016
This PR was merged into the 3.2-dev branch.

Discussion
----------

fix composer install process, resolves #20032

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #20032
| License       | MIT
| Doc PR        | reference to the documentation PR, if any

Commits
-------

f418cb8 fix composer install process, resolves #20032
@pkrupald024
Copy link

Did you try deleting the .phpunit/ folder then try again?

Thanks

@MyWhitePages

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants