You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2022/09/10 19:51:18 UTC
[GitHub] [thrift] yawaramin opened a new pull request, #2657: THRIFT-5208: fix OCaml codegen and add unit tests
yawaramin opened a new pull request, #2657:
URL: https://github.com/apache/thrift/pull/2657
My previous PR #2652 actually inadvertently broke the OCaml code generation of class names. This is a follow-up to fix both that as well as the actual issue of the exception constructor names, as well as adding actual unit tests for the OCaml codegen.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G closed pull request #2657: THRIFT-5208: fix OCaml codegen and add unit tests
Posted by GitBox <gi...@apache.org>.
Jens-G closed pull request #2657: THRIFT-5208: fix OCaml codegen and add unit tests
URL: https://github.com/apache/thrift/pull/2657
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on a diff in pull request #2657: THRIFT-5208: fix OCaml codegen and add unit tests
Posted by GitBox <gi...@apache.org>.
Jens-G commented on code in PR #2657:
URL: https://github.com/apache/thrift/pull/2657#discussion_r967704577
##########
compiler/cpp/tests/ocaml/snapshot_typedefs.cc:
##########
@@ -0,0 +1,5 @@
+const char* snapshot(R"""(
Review Comment:
missing ASF header (in most added files actually). Also, possible to squash everything into one commit for the PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] yawaramin commented on pull request #2657: THRIFT-5208: fix OCaml codegen and add unit tests
Posted by GitBox <gi...@apache.org>.
yawaramin commented on PR #2657:
URL: https://github.com/apache/thrift/pull/2657#issuecomment-1243039996
Thanks for merging. After asking for advice on how to go about writing tests in the original PR, I did not receive a response so decided to try to come up with something myself.
Most of the line count you're seeing here is from capturing snapshots of the generated OCaml code under various scenarios. I tried to keep the scenarios as simple as possible however the outputs of the service client implementations are verbose by their very nature.
These snapshots will in the future help to prevent regressions if someone makes a mistaken change in the OCaml generator as I had done previously.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on pull request #2657: THRIFT-5208: fix OCaml codegen and add unit tests
Posted by GitBox <gi...@apache.org>.
Jens-G commented on PR #2657:
URL: https://github.com/apache/thrift/pull/2657#issuecomment-1243037868
merged in [b17672a](https://github.com/apache/thrift/commit/b17672ac38cbc9347e1331798f4e6bc91502fdd2)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] yawaramin commented on a diff in pull request #2657: THRIFT-5208: fix OCaml codegen and add unit tests
Posted by GitBox <gi...@apache.org>.
yawaramin commented on code in PR #2657:
URL: https://github.com/apache/thrift/pull/2657#discussion_r967707274
##########
compiler/cpp/tests/ocaml/snapshot_service_handle_ex.cc:
##########
@@ -0,0 +1,161 @@
+// Licensed to the Apache Software Foundation(ASF) under one
+// or more contributor license agreements.See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+const char* snapshot(R"""(
+open Thrift
+open Service_types
+
+(* HELPER FUNCTIONS AND STRUCTURES *)
+
+class ping_args =
+object (self)
+ method copy =
+ let _new = Oo.copy self in
+ _new
+ method write (oprot : Protocol.t) =
+ oprot#writeStructBegin "ping_args";
+ oprot#writeFieldStop;
+ oprot#writeStructEnd
+end
+let rec read_ping_args (iprot : Protocol.t) =
+ let _str2 = new ping_args in
+ ignore(iprot#readStructBegin);
+ (try while true do
+ let (_,_t3,_id4) = iprot#readFieldBegin in
+ if _t3 = Protocol.T_STOP then
+ raise Break
+ else ();
+ (match _id4 with
+ | _ -> iprot#skip _t3);
+ iprot#readFieldEnd;
+ done; ()
+ with Break -> ());
+ iprot#readStructEnd;
+ _str2
+
+class ping_result =
+object (self)
+ val mutable _serverError : Errors_types.serverError option = None
+ method get_serverError = _serverError
+ method grab_serverError = match _serverError with None->raise (Field_empty "ping_result.serverError") | Some _x5 -> _x5
+ method set_serverError _x5 = _serverError <- Some _x5
+ method unset_serverError = _serverError <- None
+ method reset_serverError = _serverError <- None
+
+ method copy =
+ let _new = Oo.copy self in
+ if _serverError <> None then
+ _new#set_serverError self#grab_serverError#copy;
+ _new
+ method write (oprot : Protocol.t) =
+ oprot#writeStructBegin "ping_result";
+ (match _serverError with None -> () | Some _v ->
+ oprot#writeFieldBegin("serverError",Protocol.T_STRUCT,1);
+ _v#write(oprot);
+ oprot#writeFieldEnd
+ );
+ oprot#writeFieldStop;
+ oprot#writeStructEnd
+end
+let rec read_ping_result (iprot : Protocol.t) =
+ let _str8 = new ping_result in
+ ignore(iprot#readStructBegin);
+ (try while true do
+ let (_,_t9,_id10) = iprot#readFieldBegin in
+ if _t9 = Protocol.T_STOP then
+ raise Break
+ else ();
+ (match _id10 with
+ | 1 -> (if _t9 = Protocol.T_STRUCT then
+ _str8#set_serverError (Errors_types.read_serverError iprot)
+ else
+ iprot#skip _t9)
+ | _ -> iprot#skip _t9);
+ iprot#readFieldEnd;
+ done; ()
+ with Break -> ());
+ iprot#readStructEnd;
+ _str8
+
+class virtual iface =
+object (self)
+ method virtual ping : unit
+end
+
+class client (iprot : Protocol.t) (oprot : Protocol.t) =
+object (self)
+ val mutable seqid = 0
+ method ping =
+ self#send_ping;
+ self#recv_ping
+ method private send_ping =
+ oprot#writeMessageBegin ("ping", Protocol.CALL, seqid);
+ let args = new ping_args in
+ args#write oprot;
+ oprot#writeMessageEnd;
+ oprot#getTransport#flush
+ method private recv_ping =
+ let (fname, mtype, rseqid) = iprot#readMessageBegin in
+ (if mtype = Protocol.EXCEPTION then
+ let x = Application_Exn.read iprot in
+ (iprot#readMessageEnd; raise (Application_Exn.E x))
+ else ());
+ let result = read_ping_result iprot in
+ iprot#readMessageEnd;
+ (match result#get_serverError with None -> () | Some _v ->
+ raise (Errors_types.serverError _v));
+ ()
+end
+
+class processor (handler : iface) =
+object (self)
+ inherit Processor.t
+
+ val processMap = Hashtbl.create 1
+ method process iprot oprot =
+ let (name, typ, seqid) = iprot#readMessageBegin in
+ if Hashtbl.mem processMap name then
+ (Hashtbl.find processMap name) (seqid, iprot, oprot)
+ else (
+ iprot#skip(Protocol.T_STRUCT);
+ iprot#readMessageEnd;
+ let x = Application_Exn.create Application_Exn.UNKNOWN_METHOD ("Unknown function "^name) in
+ oprot#writeMessageBegin(name, Protocol.EXCEPTION, seqid);
+ x#write oprot;
+ oprot#writeMessageEnd;
+ oprot#getTransport#flush
+ );
+ true
+ method private process_ping (seqid, iprot, oprot) =
+ let _ = read_ping_args iprot in
+ iprot#readMessageEnd;
+ let result = new ping_result in
+ (try
+ (handler#ping);
+ with
+ | Errors_types.ServerError serverError ->
Review Comment:
This is where the syntax error would happen previously, e.g.:
```ocaml
| Errors_types.serverError serverError ->
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on pull request #2657: THRIFT-5208: fix OCaml codegen and add unit tests
Posted by GitBox <gi...@apache.org>.
Jens-G commented on PR #2657:
URL: https://github.com/apache/thrift/pull/2657#issuecomment-1243032545
Every time I look into this the code changes grow by 500%, What is all that "compiler/tests" stuff about? Why do we need that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] yawaramin commented on a diff in pull request #2657: THRIFT-5208: fix OCaml codegen and add unit tests
Posted by GitBox <gi...@apache.org>.
yawaramin commented on code in PR #2657:
URL: https://github.com/apache/thrift/pull/2657#discussion_r967852434
##########
compiler/cpp/tests/ocaml/snapshot_service_handle_ex.cc:
##########
@@ -0,0 +1,161 @@
+// Licensed to the Apache Software Foundation(ASF) under one
+// or more contributor license agreements.See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+const char* snapshot(R"""(
+open Thrift
+open Service_types
+
+(* HELPER FUNCTIONS AND STRUCTURES *)
+
+class ping_args =
+object (self)
+ method copy =
+ let _new = Oo.copy self in
+ _new
+ method write (oprot : Protocol.t) =
+ oprot#writeStructBegin "ping_args";
+ oprot#writeFieldStop;
+ oprot#writeStructEnd
+end
+let rec read_ping_args (iprot : Protocol.t) =
+ let _str2 = new ping_args in
+ ignore(iprot#readStructBegin);
+ (try while true do
+ let (_,_t3,_id4) = iprot#readFieldBegin in
+ if _t3 = Protocol.T_STOP then
+ raise Break
+ else ();
+ (match _id4 with
+ | _ -> iprot#skip _t3);
+ iprot#readFieldEnd;
+ done; ()
+ with Break -> ());
+ iprot#readStructEnd;
+ _str2
+
+class ping_result =
+object (self)
+ val mutable _serverError : Errors_types.serverError option = None
+ method get_serverError = _serverError
+ method grab_serverError = match _serverError with None->raise (Field_empty "ping_result.serverError") | Some _x5 -> _x5
+ method set_serverError _x5 = _serverError <- Some _x5
+ method unset_serverError = _serverError <- None
+ method reset_serverError = _serverError <- None
+
+ method copy =
+ let _new = Oo.copy self in
+ if _serverError <> None then
+ _new#set_serverError self#grab_serverError#copy;
+ _new
+ method write (oprot : Protocol.t) =
+ oprot#writeStructBegin "ping_result";
+ (match _serverError with None -> () | Some _v ->
+ oprot#writeFieldBegin("serverError",Protocol.T_STRUCT,1);
+ _v#write(oprot);
+ oprot#writeFieldEnd
+ );
+ oprot#writeFieldStop;
+ oprot#writeStructEnd
+end
+let rec read_ping_result (iprot : Protocol.t) =
+ let _str8 = new ping_result in
+ ignore(iprot#readStructBegin);
+ (try while true do
+ let (_,_t9,_id10) = iprot#readFieldBegin in
+ if _t9 = Protocol.T_STOP then
+ raise Break
+ else ();
+ (match _id10 with
+ | 1 -> (if _t9 = Protocol.T_STRUCT then
+ _str8#set_serverError (Errors_types.read_serverError iprot)
+ else
+ iprot#skip _t9)
+ | _ -> iprot#skip _t9);
+ iprot#readFieldEnd;
+ done; ()
+ with Break -> ());
+ iprot#readStructEnd;
+ _str8
+
+class virtual iface =
+object (self)
+ method virtual ping : unit
+end
+
+class client (iprot : Protocol.t) (oprot : Protocol.t) =
+object (self)
+ val mutable seqid = 0
+ method ping =
+ self#send_ping;
+ self#recv_ping
+ method private send_ping =
+ oprot#writeMessageBegin ("ping", Protocol.CALL, seqid);
+ let args = new ping_args in
+ args#write oprot;
+ oprot#writeMessageEnd;
+ oprot#getTransport#flush
+ method private recv_ping =
+ let (fname, mtype, rseqid) = iprot#readMessageBegin in
+ (if mtype = Protocol.EXCEPTION then
+ let x = Application_Exn.read iprot in
+ (iprot#readMessageEnd; raise (Application_Exn.E x))
+ else ());
+ let result = read_ping_result iprot in
+ iprot#readMessageEnd;
+ (match result#get_serverError with None -> () | Some _v ->
+ raise (Errors_types.ServerError _v));
Review Comment:
Also here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] yawaramin commented on pull request #2657: THRIFT-5208: fix OCaml codegen and add unit tests
Posted by GitBox <gi...@apache.org>.
yawaramin commented on PR #2657:
URL: https://github.com/apache/thrift/pull/2657#issuecomment-1242810108
E.g. a failing test output looks like this:
```
$ rm -rf gen-ocaml; cmake --build . && ctest --output-on-failure
[ 27%] Built target parse
[ 77%] Built target thrift_compiler
[ 83%] Building CXX object CMakeFiles/thrift_compiler_tests.dir/ocaml/snapshot_typedefs.cc.o
[ 88%] Building CXX object CMakeFiles/thrift_compiler_tests.dir/ocaml/t_ocaml_generator_tests.cc.o
[ 94%] Linking CXX executable bin/thrift_compiler_tests
[100%] Built target thrift_compiler_tests
Test project /Users/yawar/src/github.com/yawaramin/thrift/compiler/cpp/tests
Start 1: ThriftTests
1/1 Test #1: ThriftTests ......................***Failed 0.18 sec
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
thrift_compiler_tests is a Catch v1.9.4 host application.
Run with -? for options
-------------------------------------------------------------------------------
t_ocaml_generator - typedefs
-------------------------------------------------------------------------------
/Users/yawar/src/github.com/yawaramin/thrift/compiler/cpp/tests/ocaml/t_ocaml_generator_tests.cc:63
...............................................................................
/Users/yawar/src/github.com/yawaramin/thrift/compiler/cpp/tests/ocaml/t_ocaml_generator_tests.cc:75: FAILED:
REQUIRE( snapshot == gen.types() )
with expansion:
"
open Thrift
type decimal = int
"
==
"
open Thrift
type decimal = string
"
===============================================================================
test cases: 2 | 1 passed | 1 failed
assertions: 3 | 2 passed | 1 failed
0% tests passed, 1 tests failed out of 1
Total Test time (real) = 0.19 sec
The following tests FAILED:
1 - ThriftTests (Failed)
Errors while running CTest
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] yawaramin commented on a diff in pull request #2657: THRIFT-5208: fix OCaml codegen and add unit tests
Posted by GitBox <gi...@apache.org>.
yawaramin commented on code in PR #2657:
URL: https://github.com/apache/thrift/pull/2657#discussion_r967706875
##########
compiler/cpp/tests/ocaml/snapshot_typedefs.cc:
##########
@@ -0,0 +1,5 @@
+const char* snapshot(R"""(
Review Comment:
Sure, added header and squashed into a single commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on a diff in pull request #2657: THRIFT-5208: fix OCaml codegen and add unit tests
Posted by GitBox <gi...@apache.org>.
Jens-G commented on code in PR #2657:
URL: https://github.com/apache/thrift/pull/2657#discussion_r967704577
##########
compiler/cpp/tests/ocaml/snapshot_typedefs.cc:
##########
@@ -0,0 +1,5 @@
+const char* snapshot(R"""(
Review Comment:
missing ASF header (in most added files actually)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org