-
Notifications
You must be signed in to change notification settings - Fork 60
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
api: add ability to mock connections in tests #363
api: add ability to mock connections in tests #363
Conversation
0ee8687
to
4b9b201
Compare
1be6374
to
de97a4f
Compare
c9fa63b
to
a4c84d3
Compare
d78087f
to
dde51ea
Compare
b6cab56
to
0b7b2ca
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.
Thank you for the patchset. I didn't look at the code and tests very carefully, we'll leave that for the next review iteration.
It looks fine in general. So we need to do:
- Add CHANGELOG.md entry.
- Update README.md main example.
- Update README.md migration section.
- Add comments to all public API methods/types (include test_helpers).
- Add tests for new types/methods. I suppose it would be at least a response types + CreateResponse method in all requests.
d68febb
to
5cf1cff
Compare
Updated README and CHANGELOG, added more comments and more tests (I will squash commits at the last step). |
1bda50d
to
64c9b58
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.
I had partially reviewed this PR. I tried to use by commit approach (since there is a lot of changes), but it seems like some things are not where they should be. I understand that it's really a bother, but it would be nice to see a proper changes separation by commits to improve reviewing experience.
example_test.go
Outdated
// ExampleConnection_Do_failure demonstrates how to send a request and process | ||
// failure. | ||
func ExampleConnection_Do_failure() { |
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.
What's about this one?
0c29003
to
dbbf383
Compare
Updated the commits so the changes got separated properly. |
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.
Thank you for the updates! I had left a couple of comments, should be LGTM after resolving them.
example_test.go
Outdated
if resp.Header().Error != tarantool.ErrorNo { | ||
fmt.Printf("response error code: %s\n", resp.Header().Error) | ||
} |
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.
"else print success" sounds even better, since now it's always the success
dbbf383
to
9d46839
Compare
Updated the commits so the changes in |
CHANGELOG.md
Outdated
@@ -28,6 +28,19 @@ Versioning](http://semver.org/spec/v2.0.0.html) except to the first release. | |||
in requests instead of their IDs. | |||
- `GetSchema` function to get the actual schema (#7) | |||
- Support connection via an existing socket fd (#321) | |||
<<<<<<< HEAD |
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 reabase artifact.
README.md
Outdated
@@ -201,12 +202,24 @@ The subpackage has been deleted. You could use `pool` instead. | |||
unique string ID, which allows them to be distinguished. | |||
* `pool.GetPoolInfo` has been renamed to `pool.GetInfo`. Return type has been changed | |||
to `map[string]ConnectionInfo`. | |||
* Operations `Ping`, `Select`, `Insert`, `Replace`, `Delete`, `Update`, `Upsert`, | |||
<<<<<<< HEAD |
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.
And here
9d46839
to
274676b
Compare
274676b
to
cc1d250
Compare
Updated |
e3ec602
to
d143165
Compare
This commit creates a new `Response` interface. But still, only one `Response` implementation exists: `ConnResponse`. Custom responses (including mocks) are expected to implement this interface. Create a `Response` interface and its implementation `ConnResponse`. For the `Future` method `Get` now returns response data. To get the actual response new method `GetResponse` is added. `IsPush()` method is added to the response iterator. It returns the information if the current response is a `Push`. Right now it does not have a lot of usage, but it will be crucial once we create a separate response for pushes. Part of #237
Different implementations of the `Response` interface created. Special types of responses are used with special requests. Thus `Response` interface was simplified: some special methods were moved to the corresponding implementations. This means that if a user wants to access this methods, the response should be casted to its actual type. `SelectResponse`, `ExecuteResponse`, `PrepareResponse`, `PushResponse` are part of a public API. `Pos()`, `MetaData()`, `SQLInfo()` methods created for them to get specific info. `Future` constructors now accept `Request` as their argument. `Future` methods `AppendPush` and `SetResponse` accept response `Header` and data as their arguments. `IsPush()` method is used to return the information if the current response is a `PushResponse`. `PushCode` constant is removed. To get information, if the current response is a push response, `IsPush()` method could be used instead. After this patch, operations `Ping`, `Select`, `Insert`, `Replace`, `Delete`, `Update`, `Upsert`, `Call`, `Call16`, `Call17`, `Eval`, `Execute` of a `Connector` return response data instead of an actual responses. After this patch, operations `Ping`, `Select`, `Insert`, `Replace`, `Delete`, `Update`, `Upsert`, `Call`, `Call16`, `Call17`, `Eval`, `Execute` of a `Pooler` return response data instead of an actual responses. Part of #237
a4c3608
to
33a77dd
Compare
Create a mock implementations `MockRequest`, `MockResponse` and `MockDoer`. The last one allows to mock not the full `Connection`, but its part -- a structure, that implements new `Doer` interface (a `Do` function). Also added missing comments, all the changes are recorded in the `CHANGELOG` and `README` files. Added new tests and examples. So this entity is easier to implement and it is enough to mock tests that require working `Connection`. All new mock structs and an example for `MockDoer` usage are added to the `test_helpers` package. Added new structs `MockDoer`, `MockRequest` to `test_helpers`. Renamed `StrangerResponse` to `MockResponse`. Added new connection log constant: `LogAppendPushFailed`. It is logged when connection fails to append a push response. Closes #237
33a77dd
to
63e64fa
Compare
1.10.14 build is flacky on macOS-12 runner. Bump to 1.10.15 hopefully helps with this issue.
63e64fa
to
9a1b8e4
Compare
Added an ability to mock connections via new
MockRequest
,MockResponse
andMockDoer
structs. Huge rework for the responses was done in the progress, here is list of changes for every commit (3 commits total):First commit creates a new
Response
interface. But still, only oneResponse
implementation exists:ConnResponse
. Custom responses (including mocks) are expected to implement this interface.Create a
Response
interface and its implementationConnResponse
.For the
Future
methodGet
now returns response data. To get the actual response new methodGetResponse
is added.IsPush()
method is added to the response iterator. It returns the information if the current response is aPush
. Right now it does not have a lot of usage, but it will be crucial once we create a separate response for pushesSecond commit creates different implementations of the
Response
interface. Special types of responses are used with special requests.Thus
Response
interface was simplified: some special methods were moved to the corresponding implementations. This means that if a user wants to access this methods, the response should be casted to its actual type.SelectResponse
,ExecuteResponse
,PrepareResponse
,PushResponse
are part of a public API.Pos()
,MetaData()
,SQLInfo()
methods created for them to get specific info.Future
constructors now acceptRequest
as their argument.Future
methodsAppendPush
andSetResponse
accept responseHeader
and data as their arguments.IsPush()
method is used to return the information if the current response is aPushResponse
.PushCode
constant is removed. To get information, if the current response is a push response,IsPush()
method could be used instead.After this patch, operations
Ping
,Select
,Insert
,Replace
,Delete
,Update
,Upsert
,Call
,Call16
,Call17
,Eval
,Execute
of aConnector
return response data instead of an actual responses.After this patch, operations
Ping
,Select
,Insert
,Replace
,Delete
,Update
,Upsert
,Call
,Call16
,Call17
,Eval
,Execute
of aPooler
return response data instead of an actual responses.Third commit creates a mock implementations
MockRequest
,MockResponse
andMockDoer
.The last one allows to mock not the full
Connection
, but its part -- a structure, that implements newDoer
interface (aDo
function).Also added missing comments, all the changes are recorded in the
CHANGELOG
andREADME
files. Added new tests and examples.So this entity is easier to implement and it is enough to mock tests that require working
Connection
. All new mock structs and an example forMockDoer
usage are added to thetest_helpers
package.Added new structs
MockDoer
,MockRequest
totest_helpers
.Renamed
StrangerResponse
toMockResponse
.Added new connection log constant:
LogAppendPushFailed
. It is logged when connection fails to append a push response.Closes #237
I didn't forget about (remove if it is not applicable):