Add a test case to show #10348 has been fixed #10732
Conversation
mpdude
changed the title
mpdude
force-pushed
the
10348-fixed
branch
2 times, most recently
from
0a0e866
to
2307be9
Compare
|
@greg0ire Thank you for the suggestion! That makes it a lot better. I've re-checked that this test indeed fails on 2.16, so it shows the issue has been fixed. However, I wonder how we could make the test a bit more "forcing" – that is, make it obvious that naturally the ORM would delete Any ideas? |
| $this->_em->flush(); | ||
|
|
||
| self::assertEmpty($this->_em->createQuery('SELECT c FROM ' . GH10348Company::class . ' c')->getResult()); | ||
| self::assertEmpty($this->_em->createQuery('SELECT p FROM ' . GH10348Person::class . ' p')->getResult()); |
There was a problem hiding this comment.
Are these assertions maybe superfluous and distracting? We're not testing the cascade here, are we? I think it would be best to remove them and use @doesNotPerformAssertions
There was a problem hiding this comment.
In another test that we merged recently Claudio suggested to have a check like this instead of @doesNotPerformAssertions. After all, the query returning an emtpy result is the best indication of things actually happening (entities being deleted).
I'm indifferent – you decide.
There was a problem hiding this comment.
I see, we're not only checking there is no crash, we are checking that the reason there is no crash isn't nothing actually happening. Kind of far-fetched because I would expect that to be fairly covered already, but who knows. Works for me I suppose.
This is part of the series of issues fixed by doctrine#10547. In particular, the changes from doctrine#10566 were relevant. See doctrine#10348 for the bug description. Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>
|
Thanks @mpdude ! |
This is part of the series of issues fixed by #10547. In particular, the changes from #10566 were relevant.
See #10348 for the bug description.