ARROW-5875: [FlightRPC] integration tests for Flight features #6617
Conversation
lidavidm
force-pushed
the
flight-integration-minimal
branch
from
e7290d1 to
235c878
Compare
| namespace arrow { | ||
| namespace flight { | ||
|
|
||
| Status AssertEqual(const arrow::Schema& expected, const arrow::Schema& actual) { |
There was a problem hiding this comment.
Using AssertSchemaEqual if you don't mind linking to libarrow_testing?
There was a problem hiding this comment.
I realized this isn't used even, so I've deleted it.
|
|
||
| Status MakeClient(FlightClientOptions* options) override { return Status::OK(); } | ||
|
|
||
| Status RunClient(const Location& location, |
There was a problem hiding this comment.
location is not used -- what is it for?
There was a problem hiding this comment.
It was used in test_integration_client in case the server didn't return any locations in an endpoint - I've now fixed that in the server.
| if (result) { | ||
| return Status::Invalid("Action result stream has too many entries"); | ||
| } | ||
| return Status::OK(); |
There was a problem hiding this comment.
You could make a helper function to validate the results of DoAction like
Status CheckActionResults(std::vector<std::string>> results, ResultStream* stream);
There was a problem hiding this comment.
You could even run the DoAction inside the helper function
There was a problem hiding this comment.
Good idea, I've moved this into a helper function as suggested.
| arrow::Status MakeServer(std::unique_ptr<arrow::flight::FlightServerBase>* server, | ||
| arrow::flight::FlightServerOptions* options) override { | ||
| (void)server; | ||
| (void)options; |
| Status RunClient(const Location& location, | ||
| std::unique_ptr<FlightClient> client) override { | ||
| (void)location; | ||
| (void)client; |
| def __new__(cls, name, description, skip=None): | ||
| skip = skip or set() | ||
| return super().__new__( | ||
| cls, name=name, description=description, skip=skip) |
There was a problem hiding this comment.
Unless you expect this to be used in performance/memory use sensitive contexts, probably a "boring" object would be clearer:
class Scenario:
"""
An integration test scenario for Arrow Flight.
Does not correspond to a particular IPC JSON file.
"""
def __init__(self, name, description, skip=None):
self.name = name
self.description = description
self.skip = skip or set()There was a problem hiding this comment.
Good point, I've made this a normal object.
lidavidm
force-pushed
the
flight-integration-minimal
branch
2 times, most recently
from
b5afe3e to
2811a54
Compare
|
Looks good to me. The only thing I didn't quite understand was the change to |
|
Hmm, that is mostly a cosmetic change bundled in to allow you to override the response code/message, but I can remove the RuntimeException catch as that's unnecessary/too broad. |
|
Agreed, that also keeps the behaviour roughly as before (minus the extra info catches) |
lidavidm
force-pushed
the
flight-integration-minimal
branch
from
2811a54 to
8fac39b
Compare
|
Alright, I've removed the extra catch block from the Java side. |
|
👍 looks good to me! |
lidavidm
force-pushed
the
flight-integration-minimal
branch
from
8fac39b to
b7f52b6
Compare
|
I've rebased this to fix merge conflicts. Looks like the Ruby build fails in an unrelated issue, trying to fetch something from sourceforge to build docs. |
|
@wesm is this good to merge from your side? |
lidavidm
force-pushed
the
flight-integration-minimal
branch
2 times, most recently
from
50707c2 to
0ebae59
Compare
|
Rebased and fixed deprecations. |
lidavidm
force-pushed
the
flight-integration-minimal
branch
from
0ebae59 to
31784d1
Compare
|
Rebased again to fix merge conflicts. |
|
+1, sorry for the delay in merging this |
This is only a minimal test (currently the basic auth protobuf). I have more tests, but they require other fixes, so I'd rather get the basic framework down and then add to it, over creating an enormous review.