-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Throw exception NotSupported Exception for UuidGenerator
with doctr…
#8898
Throw exception NotSupported Exception for UuidGenerator
with doctr…
#8898
Conversation
Throwing as early as possible is indeed a good idea 👍 |
5c4d182
to
29ee927
Compare
Thanks @greg0ire! It was completly wrong. I've added Unit Tests to cover its logic. |
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.
Small adjustment to the exception message, otherwise good to go for me.
6920251
to
2b2f99a
Compare
2b2f99a
to
90d6835
Compare
90d6835
to
d4363ea
Compare
d4363ea
to
f717029
Compare
@@ -40,14 +40,14 @@ jobs: | |||
|
|||
- name: "Run PHPUnit" | |||
continue-on-error: true | |||
run: "vendor/bin/phpunit -c ci/github/phpunit/sqlite.xml --coverage-clover=coverage-no-cache.xml" | |||
run: "vendor/bin/phpunit -c ci/github/phpunit/sqlite.xml --coverage-clover=coverage-no-cache.xml --exclude-group performance,locking_functional,dbal2" |
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.
@greg0ire Are performance
and locking_functional
not needed for the regular "Run PHPUnit" step?
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.
performance
is run separately, that's for sure. As for locking_functional
, it's about Gearman, so it might be great to run it someday. They are already excluded in sqlite.xml
, but I think @scyzroyck had to respecify them in order to add dbal2
to the list.
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.
We'll see how this will affects the code coverage in case some test won't be executed anymore.
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.
} | ||
|
||
/** | ||
* {@inheritDoc} | ||
* | ||
* @throws NotSupported | ||
*/ | ||
public function generate(EntityManager $em, $entity) | ||
{ |
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.
Apparently this method is no longer covered: https://app.codecov.io/gh/doctrine/orm/compare/8898/changes
Why is that?
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.
If the coverage is taken from tests with doctrine/dbal:3.0, this function will not be tested as the class itself will be never initialised.
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.
@greg0ire - it looks like the possible reason might be the same name --coverage-clover=coverage-cache.xml
used for the tests with dbal2 and dbal3.
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.
It's close to that, but that's not it, I think you can have the same name as long as it differs when it's uploaded (so the upload step needs to be modified)
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 maybe I'm mistaken, looks like I already took care of setting different names when introducing the DBAL 3 build
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.
The list can be seen here: https://github.com/doctrine/orm/pull/8898/checks?check_run_id=3343305825#step:3:21
I think some might be missing indeed.
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.
Ah but the DBAL3 tests don't even run right now, so there will not be a report from them. The DBAL2 tests are running though, and are supposed to cover that path.
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.
The job supposed to cover that path are the mysql ones, and they are present… not sure what to think 🤔
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.
Independent from this PR: Shouldn't this also be covered by the other platforms?
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.
🤔 good question, might be worth removing that call to self::markTestSkipped
and see on which other platforms it passes. The situation might have evolved since this test was written in 2012.
2b90aa3
to
86b71d4
Compare
@@ -20,18 +23,28 @@ class UUIDGeneratorTest extends OrmFunctionalTestCase | |||
{ | |||
use VerifyDeprecations; | |||
|
|||
/** | |||
* @group dbal2 |
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.
Why do we need these groups anyway? Can't the same be achieved with markTestSkipped
?
The mysql jobs are passing despite me pushing a very wrong commit, so it's not a coverage upload issue, it's an execution issue. Marking as draft until this is fixed by you or me :) |
public function testGenerateUUID(): void | ||
{ | ||
if ($this->_em->getConnection()->getDatabasePlatform()->getName() !== 'mysql') { | ||
self::markTestSkipped('Currently restricted to MySQL platform.'); | ||
} | ||
|
||
if (! method_exists(Connection::class, 'getGuidExpression')) { |
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.
I don't think Connection
is the right class for this, I think you meant AbstractPlatform
. That's probably why the code was not executed.
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.
Yup, that was it!
86b71d4
to
426b426
Compare
…ine/dbal:3.x. Generating `getGuidExpression` has been removed in doctrine/dbal:3.x. Partially fixes doctrine#8884
426b426
to
cf4a4f2
Compare
Note: there is still a drop in coverage, because the DBAL 3 tests are not actually running yet (see #8886) |
Thanks @scyzoryck ! |
Let's fix another issue:
Method has been removed with doctrine/dbal#3211.
Lets throw exception if method do not exists. Generator itself is already deprecated.
Questions:
It looks like there will be few similar issues, so let's start from this one to create a template for the next PRs :)