Skip to content
This repository has been archived by the owner on Jul 28, 2022. It is now read-only.

Fix phpunit #625

Merged
merged 1 commit into from
Jun 26, 2020
Merged

Fix phpunit #625

merged 1 commit into from
Jun 26, 2020

Conversation

wbloszyk
Copy link
Member

This PR will allow run phpunit test by:

vendor/bin/simple-phpunit
bin/qa_client_ci.sh

Copy link
Member

@jordisala1991 jordisala1991 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those TODOS need to be fixed before merging tho

composer.json Outdated Show resolved Hide resolved
@wbloszyk
Copy link
Member Author

Probably fixed TODOS before merging is impossible. They depends from other sonata bundles. I can remove them but this will suspended tests. I think better keep it working becouse they show deprecations and bugs (like this TODOS) which should be fixed.

@jordisala1991
Copy link
Member

Better to have something working

@jordisala1991 jordisala1991 requested a review from a team June 17, 2020 20:37
@@ -11,10 +11,10 @@
* file that was distributed with this source code.
*/

namespace Sonata\Bundle\QABundle\Tests;
namespace Tests\Sonata\Bundle\QABundle;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefer Sonata\Tests over Tests\Sonata because the vendor is Sonata

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want create sandbox as predefine application, ready to use where people can modify them. I replace tests becouse it is default directory for it. I think will be nice use Tests\Sonata without Bundle\QABundle, mybe even split it to seperate namespace like Tests\Sonata\News. I prefer using Test\Sonata becouse people can delete them without change in composer.json (this change require use composer update).

"autoload-dev": {
    "psr-4": {
        "Tests\\": "tests/"
    }
},

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, should we merge QABundle with DemoBundle?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@core23 @wbloszyk

You can do

"autoload-dev": {
    "psr-4": {
        "Sonata\\Tests\\": "tests/"
    }
},

and then tests/Bundle/QABundle/..., without the Sonata folder.

isn't it ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want avoid modify composer.json becouse if people delete tests then they have to remove it from composer.json too and run composer update to clear it on vendor/composer files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO having standard composer .json namespaces is better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO files from vendor directory should be trait as sonata files. Files from other directory should be trait as people files to keep it as easy is possible to edit. I dont think user will want edit composer after remove files.

@VincentLanglet What when someone add own tests? Then namespace Sonata\Tests will be require.

@VincentLanglet @core23 @jordisala1991 WDYT?
My sugestion is:

  1. move AppBundle to src (App)
  2. move QABundle to DemoBundle
  3. split DemoBundle to DemoBundle and sonata-project instalator packege/bundle
  4. improve DemoBundle

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[...] if people delete tests then they have to remove it from composer.json too and run composer update to clear it on vendor/composer files.

This is a sandbox project. We should risk this and follow vendor-prefixed namespaces (Sonata\Tests).

@wbloszyk
Copy link
Member Author

RTM

@wbloszyk
Copy link
Member Author

@core23 @VincentLanglet Can you recheck it?

@jordisala1991 jordisala1991 merged commit d377075 into sonata-project:master Jun 26, 2020
@jordisala1991
Copy link
Member

Thank you @wbloszyk

@wbloszyk wbloszyk deleted the fix_phpunit branch June 30, 2020 18:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants