search

amphp / mysql

An async MySQL client for PHP, optimizing database interactions with efficient non-blocking capabilities. Perfect for responsive, high-performance applications.
Other
358 stars 63 forks source link

Better testability #63

Closed brstgt closed 6 years ago

brstgt commented 6 years ago

@trowski This issue is more a reminder or discussion than a perfectly investigated thesis.

I had in mind, you moved a lot of logic from Connection to Processor. From what I can see, the processor was meant to be a kind of state-machine to process server responses / connection states. While moving connection logic (and socket stuff) into the processor its testability suffers. With a processor that is not coupled to the IO method (connection, socket, whatever) it would be much easier to test the state machine and the protocol.

I really appreciate what you achieved with the mysql module (totally!) but I got the notion it is not really well tested or even thoroughly testable in edge cases with the current architecture. Some of the edge cases I ran into are either not easily or not at all reliably testable with a real server instance - what you obviously to in your tests. Using a module like that can be really mission critical for guys like me and if there is a problem with a release in can cause REAL trouble.

Edge case examples:

What do you think?

trowski commented 6 years ago

Processor should accept a connected Socket and use that for communication rather than creating it in connect(). Tests could then control the data on a mocked socket to test exactly the scenarios you suggested.

This package suffers a bit because it was originally written using watchers and all the horrible flow control logic that goes with that. I believe it's now at a point where I could do some further refactoring to improve testability as you mentioned, bringing it up to the standards of the other Amp packages.

bwoebi commented 6 years ago

Also: originally the Connection and Processor was a single monolith, until I realized there were some circular references.

I'm totally in favor of completely splitting the parsing part out and testing the processing of parsed data instead of having to test the protocol directly against the live mysql instance. (which is fine for integration, but can only test a small subset of the actual functionality)