-
-
Notifications
You must be signed in to change notification settings - Fork 844
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
Removed hard dependencies on doctrine odm to run the tests #2703
Conversation
Toflar
commented
Apr 6, 2019
Q | A |
---|---|
Bug fix? | no |
New feature? | no |
BC breaks? | no |
Deprecations? | no |
Tests pass? | yes |
Fixed tickets | #2484 |
License | MIT |
Doc PR |
dd6c1d1
to
b40effc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it! Many thanks for this @Toflar it wasn't an easy task haha!
WDYT @api-platform/core-team ? IMO it'd be good to have this especially that new contributors won't have to install mongodb to run the tests!
390bb64
to
64a17c7
Compare
64a17c7
to
e7f7813
Compare
Finally 😄 Cleaned up, rebased and squashed. The code coverage drop is because |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor stuff but otherwise great work! 🎉
/** | ||
* @group mongodb | ||
*/ | ||
public function testLoadDefaultConfigWithOdm() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public function testLoadDefaultConfigWithOdm() | |
public function testLoadDefaultConfigWithMongoDbOdm() |
Because there are other ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -13,7 +13,7 @@ | |||
<env name="SYMFONY_DEPRECATIONS_HELPER" value="weak_vendors" /> | |||
<server name="KERNEL_DIR" value="tests/Fixtures/app/" /> | |||
<server name="KERNEL_CLASS" value="AppKernel" /> | |||
<server name="APP_ENV" value="test_mongodb" /> | |||
<server name="APP_ENV" value="mongodb" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure it's working as expected? Because there is a specific configuration for this environment:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, tests are green so you tell me 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Toflar But the config wouldn't get applied correctly. So the 😄APP_ENV
should be reverted to test_mongodb
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or better, rename the config files, since we do the same for elasticsearch
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Toflar I think we should rename the config files instead, for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alanpoulain Can you advise why there is config_common.yml
(imported from config_orm.yml
and config_services_mongodb.yml
), config_orm.yml
(imported from config_test.yml
) and config_mongodb.yml
(I don't see where this is imported uhh, there is already a mongodb
environment previously)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note https://github.com/api-platform/core/blob/v2.4.2/tests/Fixtures/app/AppKernel.php#L97
It's very confusing if we name config files the same way when it's not environment-specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay @Toflar, so my suggestion is that we should merge mongodb
and test_mongodb
environments into one mongodb
environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's what I was trying 😄 So I revert the latest commit again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes... But we need to make sure the relevant parts of the old test_mongodb
environment are merged in, e.g.:
tests/Fixtures/app/config/config_test_mongodb.yml
tests/Fixtures/app/config/config_services_mongodb.yml
tests/Fixtures/app/config/routing_test_mongodb.yml
And the old files removed.
tests/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtensionTest.php
Outdated
Show resolved
Hide resolved
tests/Bridge/Symfony/Bundle/DependencyInjection/ConfigurationTest.php
Outdated
Show resolved
Hide resolved
Co-Authored-By: Toflar <yanick.witschi@terminal42.ch>
…ensionTest.php Co-Authored-By: Toflar <yanick.witschi@terminal42.ch>
This reverts commit 5780260.
There we go. Ping @teohhanhui :) The code coverage drop is related to the fact that the mongodb testsuite is now separated so... |
I don't understand why you asked this modification @teohhanhui. |
Yes, I understand, but we don't need two environments when one is enough. If one environment can be reused for both, it simplifies things.
It's just one |
lmao what's the status on these discussion guys? I'd like to merge this asap because it's really good for DX. |
Imho it's ready, I don't know what Teoh and Alan want me to change. We can also get this sorted on Slack maybe? |
I understand the POV of @teohhanhui but we need to be uniform then. Why having a |
Oh, that's a separate issue about the config file naming. I don't think we care about that in this PR. The main thing I asked for is one environment for mongodb, and as far as I could tell, that's already done.
I can take care of that in another PR. We need to do the same for elasticsearch, right? |
Thanks @Toflar! 🎉 🚀 |
❤️ 🚀 |