From f680e95f19f54fa287c56668d215448489352921 Mon Sep 17 00:00:00 2001
From: Henrique <henrique@henrique-vb.(none)>
Date: Fri, 10 May 2013 23:43:12 +0200
Subject: [PATCH] THRIFT-1957 NodeJS TFramedTransport and TBufferedTransport
 read bytes as unsigned Patch: Matthew Imrie

Add method to binary.js to properly read a byte type as
signed Changed transport.js readByte methods in
TFramedBuffer and TBufferedTransport to use binary.js
readByte method Added unit test for binary.js readByte
method to binary.test.js and changed test harness to
nodeunit.
---
 lib/nodejs/lib/thrift/binary.js    |   4 +
 lib/nodejs/lib/thrift/transport.js |   4 +-
 lib/nodejs/test/binary.test.js     | 127 +++++++++++++++++------------
 test/nodejs/Makefile.am            |   8 ++
 test/nodejs/client.js              | 106 ++++++++++--------------
 5 files changed, 130 insertions(+), 119 deletions(-)

diff --git a/lib/nodejs/lib/thrift/binary.js b/lib/nodejs/lib/thrift/binary.js
index 3be4d01b..62c3f729 100644
--- a/lib/nodejs/lib/thrift/binary.js
+++ b/lib/nodejs/lib/thrift/binary.js
@@ -26,6 +26,10 @@ var POW_48 = Math.pow(2, 48)
 var POW_52 = Math.pow(2, 52)
 var POW_1022 = Math.pow(2, 1022)
 
+exports.readByte = function(byte){
+	return byte > 127 ? byte-256 : byte;
+}
+
 exports.readI16 = function(buff, off) {
   off = off || 0;
   var v = buff[off + 1];
diff --git a/lib/nodejs/lib/thrift/transport.js b/lib/nodejs/lib/thrift/transport.js
index 7d83a99f..e9183350 100644
--- a/lib/nodejs/lib/thrift/transport.js
+++ b/lib/nodejs/lib/thrift/transport.js
@@ -99,7 +99,7 @@ TFramedTransport.prototype = {
   },
 
   readByte: function() {
-    return this.inBuf[this.readPos++];
+    return binary.readByte(this.inBuf[this.readPos++]);
   },
 
   readI16: function() {
@@ -223,7 +223,7 @@ TBufferedTransport.prototype = {
 
   readByte: function() {
     this.ensureAvailable(1)
-    return this.inBuf[this.readCursor++];
+    return binary.readByte(this.inBuf[this.readCursor++]);
   },
 
   readI16: function() {
diff --git a/lib/nodejs/test/binary.test.js b/lib/nodejs/test/binary.test.js
index c6bf7c95..c7f6f72f 100644
--- a/lib/nodejs/test/binary.test.js
+++ b/lib/nodejs/test/binary.test.js
@@ -17,98 +17,117 @@
  * under the License.
  */
 
-var assert = require('assert');
+var testCase = require('nodeunit').testCase;
 var binary = require('thrift/binary');
 
-module.exports = {
-  "Should read I16": function() {
-    assert.equal(0, binary.readI16([0x00, 0x00]));
-    assert.equal(1, binary.readI16([0x00, 0x01]));
-    assert.equal(-1, binary.readI16([0xff, 0xff]));
+module.exports = testCase({
+  "Should read signed byte": function(test){
+    test.equal(1, binary.readByte([0x01]));
+    test.equal(-1, binary.readByte([0xFF]));
+    
+    test.equal(127, binary.readByte([0x7F]));
+    test.equal(-128, binary.readByte([0x80]));
+    test.done();
+  },
+  "Should write byte": function(test){
+  	//Protocol simply writes to the buffer. Nothing to test.. yet.
+  	test.ok(true);
+  	test.done();
+  },
+  "Should read I16": function(test) {
+    test.equal(0, binary.readI16([0x00, 0x00]));
+    test.equal(1, binary.readI16([0x00, 0x01]));
+    test.equal(-1, binary.readI16([0xff, 0xff]));
 
     // Min I16
-    assert.equal(-32768, binary.readI16([0x80, 0x00]));
+    test.equal(-32768, binary.readI16([0x80, 0x00]));
     // Max I16
-    assert.equal(32767, binary.readI16([0x7f, 0xff]));
+    test.equal(32767, binary.readI16([0x7f, 0xff]));
+    test.done();
   },
 
-  "Should write I16": function() {
-    assert.deepEqual([0x00, 0x00], binary.writeI16([], 0));
-    assert.deepEqual([0x00, 0x01], binary.writeI16([], 1));
-    assert.deepEqual([0xff, 0xff], binary.writeI16([], -1));
+  "Should write I16": function(test) {
+    test.deepEqual([0x00, 0x00], binary.writeI16([], 0));
+    test.deepEqual([0x00, 0x01], binary.writeI16([], 1));
+    test.deepEqual([0xff, 0xff], binary.writeI16([], -1));
 
     // Min I16
-    assert.deepEqual([0x80, 0x00], binary.writeI16([], -32768));
+    test.deepEqual([0x80, 0x00], binary.writeI16([], -32768));
     // Max I16
-    assert.deepEqual([0x7f, 0xff], binary.writeI16([], 32767));
+    test.deepEqual([0x7f, 0xff], binary.writeI16([], 32767));
+    test.done();
   },
 
-  "Should read I32": function() {
-    assert.equal(0, binary.readI32([0x00, 0x00, 0x00, 0x00]));
-    assert.equal(1, binary.readI32([0x00, 0x00, 0x00, 0x01]));
-    assert.equal(-1, binary.readI32([0xff, 0xff, 0xff, 0xff]));
+  "Should read I32": function(test) {
+    test.equal(0, binary.readI32([0x00, 0x00, 0x00, 0x00]));
+    test.equal(1, binary.readI32([0x00, 0x00, 0x00, 0x01]));
+    test.equal(-1, binary.readI32([0xff, 0xff, 0xff, 0xff]));
 
     // Min I32
-    assert.equal(-2147483648, binary.readI32([0x80, 0x00, 0x00, 0x00]));
+    test.equal(-2147483648, binary.readI32([0x80, 0x00, 0x00, 0x00]));
     // Max I32
-    assert.equal(2147483647, binary.readI32([0x7f, 0xff, 0xff, 0xff]));
+    test.equal(2147483647, binary.readI32([0x7f, 0xff, 0xff, 0xff]));
+    test.done();
   },
 
-  "Should write I32": function() {
-    assert.deepEqual([0x00, 0x00, 0x00, 0x00], binary.writeI32([], 0));
-    assert.deepEqual([0x00, 0x00, 0x00, 0x01], binary.writeI32([], 1));
-    assert.deepEqual([0xff, 0xff, 0xff, 0xff], binary.writeI32([], -1));
+  "Should write I32": function(test) {
+    test.deepEqual([0x00, 0x00, 0x00, 0x00], binary.writeI32([], 0));
+    test.deepEqual([0x00, 0x00, 0x00, 0x01], binary.writeI32([], 1));
+    test.deepEqual([0xff, 0xff, 0xff, 0xff], binary.writeI32([], -1));
 
     // Min I32
-    assert.deepEqual([0x80, 0x00, 0x00, 0x00], binary.writeI32([], -2147483648));
+    test.deepEqual([0x80, 0x00, 0x00, 0x00], binary.writeI32([], -2147483648));
     // Max I32
-    assert.deepEqual([0x7f, 0xff, 0xff, 0xff], binary.writeI32([], 2147483647));
+    test.deepEqual([0x7f, 0xff, 0xff, 0xff], binary.writeI32([], 2147483647));
+ 	test.done();
   },
 
-  "Should read doubles": function() {
-    assert.equal(0, binary.readDouble([0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
-    assert.equal(0, binary.readDouble([0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
-    assert.equal(1, binary.readDouble([0x3f, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
-    assert.equal(2, binary.readDouble([0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
-    assert.equal(-2, binary.readDouble([0xc0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
+  "Should read doubles": function(test) {
+    test.equal(0, binary.readDouble([0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
+    test.equal(0, binary.readDouble([0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
+    test.equal(1, binary.readDouble([0x3f, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
+    test.equal(2, binary.readDouble([0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
+    test.equal(-2, binary.readDouble([0xc0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
 
-    assert.equal(Math.PI, binary.readDouble([0x40, 0x9, 0x21, 0xfb, 0x54, 0x44, 0x2d, 0x18]))
+    test.equal(Math.PI, binary.readDouble([0x40, 0x9, 0x21, 0xfb, 0x54, 0x44, 0x2d, 0x18]))
 
-    assert.equal(Infinity, binary.readDouble([0x7f, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
-    assert.equal(-Infinity, binary.readDouble([0xff, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
+    test.equal(Infinity, binary.readDouble([0x7f, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
+    test.equal(-Infinity, binary.readDouble([0xff, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
 
-    assert.ok(isNaN(binary.readDouble([0x7f, 0xf8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00])))
+    test.ok(isNaN(binary.readDouble([0x7f, 0xf8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00])))
 
-    assert.equal(1/3, binary.readDouble([0x3f, 0xd5, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55]))
+    test.equal(1/3, binary.readDouble([0x3f, 0xd5, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55]))
 
     // Min subnormal positive double
-    assert.equal(4.9406564584124654e-324, binary.readDouble([0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01]))
+    test.equal(4.9406564584124654e-324, binary.readDouble([0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01]))
     // Min normal positive double
-    assert.equal(2.2250738585072014e-308, binary.readDouble([0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
+    test.equal(2.2250738585072014e-308, binary.readDouble([0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
     // Max positive double
-    assert.equal(1.7976931348623157e308, binary.readDouble([0x7f, 0xef, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff]))
+    test.equal(1.7976931348623157e308, binary.readDouble([0x7f, 0xef, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff]))
+  	test.done();
   },
 
-  "Should write doubles": function() {
-    assert.deepEqual([0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], 0));
-    assert.deepEqual([0x3f, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], 1));
-    assert.deepEqual([0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], 2));
-    assert.deepEqual([0xc0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], -2));
+  "Should write doubles": function(test) {
+    test.deepEqual([0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], 0));
+    test.deepEqual([0x3f, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], 1));
+    test.deepEqual([0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], 2));
+    test.deepEqual([0xc0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], -2));
 
-    assert.deepEqual([0x40, 0x9, 0x21, 0xfb, 0x54, 0x44, 0x2d, 0x18], binary.writeDouble([], Math.PI));
+    test.deepEqual([0x40, 0x9, 0x21, 0xfb, 0x54, 0x44, 0x2d, 0x18], binary.writeDouble([], Math.PI));
 
-    assert.deepEqual([0x7f, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], Infinity));
-    assert.deepEqual([0xff, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], -Infinity));
+    test.deepEqual([0x7f, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], Infinity));
+    test.deepEqual([0xff, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], -Infinity));
 
-    assert.deepEqual([0x7f, 0xf8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], NaN));
+    test.deepEqual([0x7f, 0xf8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], NaN));
 
-    assert.deepEqual([0x3f, 0xd5, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55], binary.writeDouble([], 1/3));
+    test.deepEqual([0x3f, 0xd5, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55], binary.writeDouble([], 1/3));
 
     // Min subnormal positive double
-    assert.deepEqual([0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01], binary.writeDouble([], 4.9406564584124654e-324)); 
+    test.deepEqual([0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01], binary.writeDouble([], 4.9406564584124654e-324)); 
     // Min normal positive double
-    assert.deepEqual([0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], 2.2250738585072014e-308)); 
+    test.deepEqual([0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], 2.2250738585072014e-308)); 
     // Max positive double
-    assert.deepEqual([0x7f, 0xef, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff], binary.writeDouble([], 1.7976931348623157e308)); 
+    test.deepEqual([0x7f, 0xef, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff], binary.writeDouble([], 1.7976931348623157e308)); 
+  	test.done();
   }
-}
+});
\ No newline at end of file
diff --git a/test/nodejs/Makefile.am b/test/nodejs/Makefile.am
index 32397f1b..8f74ba86 100755
--- a/test/nodejs/Makefile.am
+++ b/test/nodejs/Makefile.am
@@ -22,6 +22,14 @@ stubs: ../ThriftTest.thrift
 	$(THRIFT) --gen js:node ../ThriftTest.thrift
 
 check: stubs
+	@if which expresso &> /dev/null ; then \
+		echo "   Testing thrift/binary"; \
+		NODE_PATH=../../lib/nodejs/lib:../../lib/nodejs/lib/thrift:$(NODE_PATH) nodeunit ../../lib/nodejs/test/binary.test.js; \
+	fi
+	timeout 2 $(MAKE) server &
+	@sleep 1
+	$(MAKE) client
+	@sleep 1
 
 clean-local:
 	$(RM) -r gen-nodejs
diff --git a/test/nodejs/client.js b/test/nodejs/client.js
index 88460b00..3a90cac7 100644
--- a/test/nodejs/client.js
+++ b/test/nodejs/client.js
@@ -17,6 +17,7 @@
  * under the License.
  */
 var thrift = require('thrift');
+var assert = require('assert');
 
 var ThriftTest = require('./gen-nodejs/ThriftTest'),
     ttypes = require('./gen-nodejs/ThriftTest_types');
@@ -24,81 +25,61 @@ var ThriftTest = require('./gen-nodejs/ThriftTest'),
 var connection = thrift.createConnection('localhost', 9090),
     client = thrift.createClient(ThriftTest, connection);
 
-var tfailed = 0;
-var tpassed = 0;
-
-function failed(err) {
-  console.trace(err);
-	return tfailed++;
-}
-function passed() {
-	return tpassed++;
-}
-
 connection.on('error', function(err) {
-  failed(err);
+  assert(false, err);
 });
 
-console.time("Tests completed in");
+
 
 client.testVoid(function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testVoid() = ", response);
-	passed();
+  assert( ! err);
+  assert.equal(undefined, response);
 });
 
 client.testString("Test", function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testString('Test') = ", response);
-	passed();
+  assert( ! err);
+  assert.equal("Test", response);
 });
 
 client.testByte(1, function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testByte(1) = ", response);
-	passed();
+  assert( ! err);
+  assert.equal(1, response);
 });
 
 client.testI32(-1, function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testI32(-1) = ", response);
-	passed();
+  assert( ! err);
+  assert.equal(-1, response);
 });
 
 client.testI64(5, function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testI64(5) = ", response);
-	passed();
+  assert( ! err);
+  assert.equal(5, response);
 });
 
 client.testI64(-5, function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testI64(-5) = ", response);
-	passed();
+  assert( ! err);
+  assert.equal(-5, response);
 });
 
 client.testI64(-34359738368, function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testI64(-34359738368) = ", response);
-	passed();
+  assert( ! err);
+  assert.equal(-34359738368, response);
 });
 
 client.testDouble(-5.2098523, function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testDouble(-5.2098523) = ", response);
-	passed();
+  assert( ! err);
+  assert.equal(-5.2098523, response);
 });
 
 var out = new ttypes.Xtruct({
-	string_thing: 'Zero',
-	byte_thing: 1,
-	i32_thing: -3,
-	i64_thing: 1000000
+  string_thing: 'Zero',
+  byte_thing: 1,
+  i32_thing: -3,
+  //i64_thing: 1000000
 });
 client.testStruct(out, function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testStruct(", out, ") = \n", response);
-	passed();
+  assert( ! err);
+  assert.deepEqual(out, response);
 });
 
 var out2 = new ttypes.Xtruct2();
@@ -106,9 +87,8 @@ out2.byte_thing = 1;
 out2.struct_thing = out;
 out2.i32_thing = 5;
 client.testNest(out2, function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testNest(", out2, ") = \n", response);
-	passed();
+  assert( ! err);
+  assert.deepEqual(out2, response);
 });
 
 var mapout = {};
@@ -116,9 +96,8 @@ for (var i = 0; i < 5; ++i) {
   mapout[i] = i-10;
 }
 client.testMap(mapout, function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testMap(", mapout, ") = \n", response);
-	passed();
+  assert( ! err);
+  assert.deepEqual(mapout, response);
 });
 
 /*
@@ -127,25 +106,26 @@ client.testMap(mapout, function(err, response) {
 
 
 client.testException('ApplicationException', function(err, response) {
-  console.log("testException('ApplicationException') = ", err);
-  if (response) { return failed(response); }
-	passed();
+  //assert.equal('ApplicationException', err);
+  assert( ! response);
 });
 
 client.testException('Xception', function(err, response) {
-  console.log("testException('Xception') = ", err);
-  if (response) { return failed(response); }
-	passed();
+  assert.equal('Xception', err.message);
+  assert( ! response);
 });
 
 client.testException('success', function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testException('success') = ", response);
-	passed();
+  assert( ! err);
+  //assert.equal('success', response);
 });
 
-setTimeout(function(){
-  console.timeEnd("Tests completed in");
-	console.log(tpassed + " passed, " + tfailed + " failed");
-	connection.end();
+
+setTimeout(function() {
+  console.log("Server successfully tested!");
+  connection.end();
 }, 200);
+
+// to make it also run on expresso
+exports.expressoTest = function() {};
+
-- 
2.17.1