-
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
Replace mocks with middlewares #9291
Conversation
protected function setUp(): void | ||
{ | ||
$this->idMocker = new LastInsertIdMocker(); | ||
$config = new Configuration(); | ||
$config->setMiddlewares([new LastInsertIdMockMiddleware($this->idMocker)]); |
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.
This still looks like too much code to mock a single value returned by Connection::lastInsertId()
. Would it make sense to use PHPUnit mocks for that? Or the test uses other Connection APIs for which we want to use the default implementation?
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, the test persists entities in a real database. What we want to test is if the ORM handles very large IDs returned as string correctly. If you have an idea to simplify this partial mock setup, I'm all ears.
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 is indeed a functional test. In this case, it shouldn't use mocks. It should reproduce a real case instead. BTW, what is the corresponding code change that it covers?
For instance, initialize the autoincrement field with PHP_INT_MAX
and insert a new row.
Alternatively, since it's not a real functional test, it could be converted into a unit one and cover only the corresponding component w/o mocking the whole world.
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 is indeed a functional test. In this case, it shouldn't use mocks. It should reproduce a real case instead. BTW, what is the corresponding code change that it covers?
That would be this seven years old PR: #1346.
For instance, initialize the autoincrement field with
PHP_INT_MAX
and insert a new row.
Does the schema manager give me a nice way to do that?
Alternatively, since it's not a real functional test, it could be converted into a unit one and cover only the corresponding component w/o mocking the whole world.
Yes, a unit test would not be a bad idea, however since we need to make sure that UoW and persisters talk to each other correctly, I would not want to remove the functional test either way.
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.
Does the schema manager give me a nice way to do that?
I don't think so, since there's no portable API for that.
Yes, a unit test would not be a bad idea, however since we need to make sure that UoW and persisters talk to each other correctly, I would not want to remove the functional test either way.
This test doesn't cover a significant part for the functionality. Specifically, the driver returning a bigint. As we learned recently, SQLite returns integers as integers. Mocking this anything in an integration test is a bit naive IMO.
FWIW, I don't mind these changes merged in any state. But IMO the test in question causes more trouble than produces value. If it was implemented properly, it wouldn't have to be updated in the first place.
c2b12c7
to
e1abaac
Compare
e1abaac
to
43ecdc3
Compare
This PR improves a test by replacing a mock of DBAL's
Connection
class by a middleware. This should make the test resilient against future DBAL changes and ease the midgration path towards DBAL 4.