liip / LiipFunctionalTestBundle

Some helper classes for writing functional tests in Symfony
http://liip.ch
MIT License
639 stars 182 forks source link

Move all files to `src` and `tests` #323

Closed ruudk closed 7 years ago

ruudk commented 7 years ago

Is it ok if I create a PR that moves all sources to src and the tests in tests? This way, the tests won't get distributed with composer.

I'm currently trying to migrate my Symfony 3.2 project to the Symfony 3.0 directory structure format and I'm getting weird errors like this:

$ composer install
...
...\Core\Composer\ScriptHandler::clearCache
PHP Fatal error:  Cannot declare class AppKernel, because the name is already in use in /vagrant/vendor/liip/functional-test-bundle/Tests/App/AppKernel.php on line 54

Fatal error: Cannot declare class AppKernel, because the name is already in use in /vagrant/vendor/liip/functional-test-bundle/Tests/App/AppKernel.php on line 54
Segmentation fault (core dumped)
Script ....\Core\Composer\ScriptHandler::clearCache handling the post-install-cmd event terminated with an exception

  [RuntimeException]
  An error occurred when executing the "'cache:clear --no-warmup'" command:
  Fatal error: Cannot declare class AppKernel, because the name is already in use in /vagrant/vendor/liip
  /functional-test-bundle/Tests/App/AppKernel.php on line 54
  PHP Fatal error:  Cannot declare class AppKernel, because the name is already in use in /vagrant/vendor
  /liip/functional-test-bundle/Tests/App/AppKernel.php on line 54
  Segmentation fault (core dumped)

install [--prefer-source] [--prefer-dist] [--dry-run] [--dev] [--no-dev] [--no-custom-installers] [--no-autoloader] [--no-scripts] [--no-progress] [--no-suggest] [-v|vv|vvv|--verbose] [-o|--optimize-autoloader] [-a|--classmap-authoritative] [--apcu-autoloader] [--ignore-platform-reqs] [--] [<packages>]...
alexislefebvre commented 7 years ago

It will probably be simpler to exclude some directories from the archive: https://stackoverflow.com/questions/17049313/how-to-ignore-directories-with-composer

ruudk commented 7 years ago

@alexislefebvre Thanks! I found that one as well, wasn't sure if it would work. Going to test it now in https://github.com/liip/LiipFunctionalTestBundle/pull/325

ruudk commented 7 years ago

@alexislefebvre It doesn't work. Still gives the same error. So I think it's better to just split it.

alexislefebvre commented 7 years ago

It will probably be simpler to exclude some directories from the archive: https://stackoverflow.com/questions/17049313/how-to-ignore-directories-with-composer

I tested it by creating a branch with a .gitattributes file on this repo: https://github.com/liip/LiipFunctionalTestBundle/commit/0039e07d009a0e3e08cf826b555c0dcee248152a

Then I installed this branch of the package:

$ composer require liip/functional-test-bundle:dev-add-dot-gitattributes-file

Unfortunately, the Tests/ directory is still here :disappointed::

$ ls vendor/liip/functional-test-bundle/Tests/
App/  AppConfig/  AppConfigLeanFramework/  AppConfigMysql/  AppConfigPhpcr/  Command/  DependencyInjection/  Test/

Maybe it's due to the fact that Composer cloned the repo instead of downloading a zip file. Maybe it works only when a tag is created.

ruudk commented 7 years ago

Why not just move it into src and tests? :) https://github.com/liip/LiipFunctionalTestBundle/pull/324

alexislefebvre commented 7 years ago

Because I'm trying to find a simpler solution before moving all the Tests/ files.

Can you please share the steps you followed in order to have the error you mentioned in the beginning of this issue?

Aerendir commented 7 years ago

If I can, I agree with @ruudk : following the standards is a mandatory step to make a bundle understandable, apart from the eventual errors that may happen, especially if the work is already done.. (#324). IMHO.

alexislefebvre commented 7 years ago

@Aerendir

following the standards

It doesn't look like moving all the files to src/ is a standard. Not all bundles use this files structure.

@lsmith77 Can you please give your opinion about these changes? What do you think of moving all the files to src and tests directories? Thanks.

Aerendir commented 7 years ago

@alexislefebvre Beh, it's clearly a Symfony's standard at least (adopted from the Unix world): http://fabien.potencier.org/symfony4-directory-structure.html

I understand not all bundles adopt this new structure, but this is not a valid reason, in my opinion, to not adopt it for LiipFunctionalTestsBundle: if we can improve, we should...

I don't touch the code of this bundle since a lot of time, and reading again the code, in the first time confused me: where should have I to look for the classes I have to use in my project?

schermata 2017-06-27 alle 11 12 11

Sure, opening each folder solves the dilemma, but... I have to open both folders first. It's not immediate.

I think that starting to use a more robust folder structure helps also to make a deeper refactoring that will make the bundle better: https://github.com/liip/LiipFunctionalTestBundle/issues/218

alexislefebvre commented 7 years ago

@Aerendir the link is about a good practice for a Symfony project (with var/, web/, etc.), not a Symfony bundle.

Anyway I agree with you @ruudk and @Aerendir that this is a common practice and we don't have any other option to avoid the bug repored in this issue. My only concern now is that it will break every PR. we have to check that this is worth the change.

Aerendir commented 7 years ago

@alexislefebvre "Everything is a bundle in Symfony" :stuck_out_tongue_closed_eyes: No, seriously, I know it is about a generic project, but I think is anyway a good practice to separate the two things: in one folder the source code, in another the tests...

I'm happy you agree :)

Aerendir commented 7 years ago

@ruudk Why did you close this issue?

ruudk commented 7 years ago

Because I don't want to pursue with the PR anymore. I got it working without.

phillipsnick commented 7 years ago

Any details on how you got it working? Having the same problem with Cannot declare class AppKernel, because the name is already in use in and can't just seem to be going in circles.

I agree that src and tests is the best approach, so much easier to follow.