ASF GitHub Bot
added a comment - 03/Jan/15 02:40 Github user radekg commented on the pull request:
https://github.com/apache/thrift/pull/345#issuecomment-68580492
This build has failed because of some jslint issues. Any hints how can I help resolving?

ASF GitHub Bot
added a comment - 03/Jan/15 08:28 Github user bufferoverflow commented on the pull request:
https://github.com/apache/thrift/pull/345#issuecomment-68587476
jslint is used via lib/js/test/build.xml which also runs the unit tests test.js/test.html
My favorite would be integration of your tests into test.js/test.html and use guessProtocolFactory within the server part https://github.com/apache/thrift/blob/master/lib/js/test/src/test/Httpd.java#L217

@bufferoverflow I am trying to understand where the problem is. It seems that travis is using a different method of testing than the one documented in the README for js tests. The README assumes grunt as a main way of testing while travis seems to be using ant?
I am also trying to figure out how does one run the main set of tests. Do I have to follow this write up https://thrift.apache.org/docs/BuildingFromSource ?
Regarding the failing tests, travis complains about some invalid bitwise operations while bitwise is allowed by build.xml jslint. Not sure where to start looking for stuff.

ASF GitHub Bot
added a comment - 05/Jan/15 10:50 Github user radekg commented on the pull request:
https://github.com/apache/thrift/pull/345#issuecomment-68693024
@bufferoverflow I am trying to understand where the problem is. It seems that travis is using a different method of testing than the one documented in the README for js tests. The README assumes grunt as a main way of testing while travis seems to be using ant?
I am also trying to figure out how does one run the main set of tests. Do I have to follow this write up https://thrift.apache.org/docs/BuildingFromSource ?
Regarding the failing tests, travis complains about some invalid bitwise operations while bitwise is allowed by build.xml jslint. Not sure where to start looking for stuff.

I've done a little bit more research into this. There's a major issue which will surface in the unit test once I can get it integrated. I've got the test properly integrated in the grunt task. What happens it that PhantomJS doesn't like the following expression:

String.fromCharCode.apply(null, new Uint8Array( buffer ) )

This has been described here: https://github.com/mozilla/pdf.js/issues/1955
The problem is, if I apply the "fix" mentioned in the comments, the test doesn't pass because unicode characters are decoded incorrectly. So, one way or another, the test will fail. I can see only one solution to this issue. I am going to close this pull request and provide the ThrifBinaryProtocol for JS as an external module to be added on top of Thrift library. Once this is done, I will post a link to the project here.

ASF GitHub Bot
added a comment - 05/Jan/15 19:04 Github user radekg commented on the pull request:
https://github.com/apache/thrift/pull/345#issuecomment-68758030
I've done a little bit more research into this. There's a major issue which will surface in the unit test once I can get it integrated. I've got the test properly integrated in the grunt task. What happens it that PhantomJS doesn't like the following expression:
String.fromCharCode.apply(null, new Uint8Array( buffer ) )
This has been described here: https://github.com/mozilla/pdf.js/issues/1955
The problem is, if I apply the "fix" mentioned in the comments, the test doesn't pass because unicode characters are decoded incorrectly. So, one way or another, the test will fail. I can see only one solution to this issue. I am going to close this pull request and provide the ThrifBinaryProtocol for JS as an external module to be added on top of Thrift library. Once this is done, I will post a link to the project here.

The JavaScript test suite is not perfectly aligned with the current Apache Thrift test strategy (THRIFT-847).

Apache Thrift Test Strategy

Apache Thrift uses "make check" to drive language by language unit tests. These test each language's implementation of Apache Thrift for internal consistency and belong in thrift/lib/lang/test. Apache Thrift "make cross" drives cross language integration tests which insure languages can communicate with each other using a range of protocols and transports. These tests are found in thrift/test, along with other shared testing components (largely thrift IDL files). As this is a work in progress, many languages have no presence in the current make cross tests and still others have skewed make check test suites.

Apache Thrift Browser JavaScript Test Structure

JavaScript tests are currently organized into two suites.

ant thrift/lib/js/test/build.xml - These test JavaScript in the browser against a Java server. These tests require Java to be installed and also require the Java libs/tests to be built. Presently, to run these tests, you can run "./bootstrap.sh; ./configure; make check" at the thrift root, or if you have already done so in the past, you can run "make check" in thrift/lib/js/test to just run the JavaScript tests. Java must be installed when you run ./configure. Note: The cmake build does not yet support "make check" (1/5/2015).

grunt thrift/lib/js/Gruntfile.js - These test JavaScript in the Browser (using Phantom as the host) against JavaScript on the server (using NodeJS as the host). All of the JavaScript build tools (principally grunt and npm) are JavaScript based and do not require Java to be installed to work, though they do require Node.js. This test suite is more comprehensive than the build.xml suite, testing SSL, various jQuery arrangements, etc.

Target JavaScript Test Structure

Given that the Ant JavaScript to Java tests are cross language, these should be moved to the make check chain (integrated with thrift/test/test.sh/py, eliminating the thrift/lib/js/test/build.xml altogether). The grunt tests should then be wired into the make check tests as the go forward unit tests.

Tests for a new protocol

Any new protocol should have standard unit tests and a full set of integration tests with at least one of the two reference implementations (CPP/Java). In the case of JavaScript, this would imply a complete set of grunt driven tests against a Node.js server (adding TBinaryProtocol to the grunt test html files) and a "make cross" integration test with CPP or Java (wherein we should add tests for TBinaryProtocol and TJSONProtocol as well).

Radoslaw Gruchalski If you can get the full suite of grunt tests (Browser to Node) to pass using TBinaryProtocol and the equivalent of the Ant tests to pass (Browser to Java) I will help you on the Apache Thrift build/test suite side.

Randy Abernethy
added a comment - 05/Jan/15 19:19 The JavaScript test suite is not perfectly aligned with the current Apache Thrift test strategy ( THRIFT-847 ).
Apache Thrift Test Strategy
Apache Thrift uses "make check" to drive language by language unit tests. These test each language's implementation of Apache Thrift for internal consistency and belong in thrift/lib/ lang /test. Apache Thrift "make cross" drives cross language integration tests which insure languages can communicate with each other using a range of protocols and transports. These tests are found in thrift/test, along with other shared testing components (largely thrift IDL files). As this is a work in progress, many languages have no presence in the current make cross tests and still others have skewed make check test suites.
Apache Thrift Browser JavaScript Test Structure
JavaScript tests are currently organized into two suites.
ant thrift/lib/js/test/build.xml - These test JavaScript in the browser against a Java server. These tests require Java to be installed and also require the Java libs/tests to be built. Presently, to run these tests, you can run "./bootstrap.sh; ./configure; make check" at the thrift root, or if you have already done so in the past, you can run "make check" in thrift/lib/js/test to just run the JavaScript tests. Java must be installed when you run ./configure. Note: The cmake build does not yet support "make check" (1/5/2015).
grunt thrift/lib/js/Gruntfile.js - These test JavaScript in the Browser (using Phantom as the host) against JavaScript on the server (using NodeJS as the host). All of the JavaScript build tools (principally grunt and npm) are JavaScript based and do not require Java to be installed to work, though they do require Node.js. This test suite is more comprehensive than the build.xml suite, testing SSL, various jQuery arrangements, etc.
Target JavaScript Test Structure
Given that the Ant JavaScript to Java tests are cross language, these should be moved to the make check chain (integrated with thrift/test/test. sh/py , eliminating the thrift/lib/js/test/build.xml altogether). The grunt tests should then be wired into the make check tests as the go forward unit tests.
Tests for a new protocol
Any new protocol should have standard unit tests and a full set of integration tests with at least one of the two reference implementations (CPP/Java). In the case of JavaScript, this would imply a complete set of grunt driven tests against a Node.js server (adding TBinaryProtocol to the grunt test html files) and a "make cross" integration test with CPP or Java (wherein we should add tests for TBinaryProtocol and TJSONProtocol as well).
Radoslaw Gruchalski If you can get the full suite of grunt tests (Browser to Node) to pass using TBinaryProtocol and the equivalent of the Ant tests to pass (Browser to Java) I will help you on the Apache Thrift build/test suite side.
Best,
Randy

I’d love to, it would be awesome to provide this to the community but the PhantomJS issue will prevent unicode characters decryption. The only way I can make this test to pass is to remove unicode characters from the test It work perfectly in the browser. I’m running this code here: https://github.com/radekg/gossiperl-client-chrome.

Confidentiality:
This communication is intended for the above-named person and may be confidential and/or legally privileged.
If it has come to you in error you must take no action based on it, nor must you copy or show it to anyone; please delete/destroy and inform the sender immediately.

Radoslaw Gruchalski
added a comment - 05/Jan/15 19:24 - edited I’d love to, it would be awesome to provide this to the community but the PhantomJS issue will prevent unicode characters decryption. The only way I can make this test to pass is to remove unicode characters from the test It work perfectly in the browser. I’m running this code here: https://github.com/radekg/gossiperl-client-chrome .
Kind regards,
Radek Gruchalski
de.linkedin.com/in/radgruchalski/ ( http://de.linkedin.com/in/radgruchalski/ )
Confidentiality:
This communication is intended for the above-named person and may be confidential and/or legally privileged.
If it has come to you in error you must take no action based on it, nor must you copy or show it to anyone; please delete/destroy and inform the sender immediately.

Radoslaw Gruchalski
added a comment - 05/Jan/15 19:31 I've committed my simple binary test which visualises the PhantomJS problem: https://github.com/radekg/thrift/commit/e74c0d7c40fe3de9312ee571990fcbac8fa28249
Not sure if this can be resolved but if so, I can find a bit of time to implement the integration test with the nodejs server.

I don't think we should let Phantom hold us back. It is not a practical environment, just a testing tool. We had to excise the WebSocket tests (testws.html) because Phantom did not support the current WebSocket spec. If you can get as many of the tests working as possible and move the others to a separate html, we can run the outliers manually until Phantom comes in line. We can also consider other options like Zombie or WebDriver.

Randy Abernethy
added a comment - 05/Jan/15 19:47 Hey Radek,
I don't think we should let Phantom hold us back. It is not a practical environment, just a testing tool. We had to excise the WebSocket tests (testws.html) because Phantom did not support the current WebSocket spec. If you can get as many of the tests working as possible and move the others to a separate html, we can run the outliers manually until Phantom comes in line. We can also consider other options like Zombie or WebDriver.
-Randy

Radoslaw Gruchalski
added a comment - 05/Jan/15 20:00 Randy, just to clarify - these tests are passing in the browser. Feel free to try from my branch. What would be the absolute minimum to get this in?

Where are we with this patch? I just tried to commit but the qunit:ThriftBinaryProtocol test still fails. I cleaned up the white space and did a subst to change the proto from BinaryProtocol to TBinaryProtocol.

Also tried the UTF-8 exports Roger mentions.

Radoslaw Gruchalski if you can mod the phantom tests to leave out the strings so that they pass and leave the full tests for manual browser use we can get this committed.

Randy Abernethy
added a comment - 31/Jan/15 15:11 Hey Guys,
Where are we with this patch? I just tried to commit but the qunit:ThriftBinaryProtocol test still fails. I cleaned up the white space and did a subst to change the proto from BinaryProtocol to TBinaryProtocol.
Also tried the UTF-8 exports Roger mentions.
Radoslaw Gruchalski if you can mod the phantom tests to leave out the strings so that they pass and leave the full tests for manual browser use we can get this committed.
Best,
Randy

FWIW, this looks to only apply to the js implementation in lib/js. My intent is to get the nodejs code all working in the browser via browserify. I've already been able to implement this and test successfully (in this proof of concept commit**) in phantomjs. The only tests I cannot successfully run in phantomjs are the websockets tests because phantomjs 1.x.x doesn't implement the correct websocket specifications, but phantomjs 2 does and should be released shortly. The XHR proof of concept works flawlessly in phantomjs (I am using run-browser https://github.com/ForbesLindesay/run-browser ).

If we get all the nodejs code working well in the browser, it's entirely possible that we could completely deprecate the implementation in lib/js unless someone has a strong reason to maintain that implementation. It's trivial to take the nodejs browser capable implementation and package it up so it produces a standalone file that exposes thrift as a global on the window object if that is something people want.

Andrew de Andrade
added a comment - 01/Feb/15 01:50 FWIW, this looks to only apply to the js implementation in lib/js. My intent is to get the nodejs code all working in the browser via browserify. I've already been able to implement this and test successfully (in this proof of concept commit**) in phantomjs. The only tests I cannot successfully run in phantomjs are the websockets tests because phantomjs 1.x.x doesn't implement the correct websocket specifications, but phantomjs 2 does and should be released shortly. The XHR proof of concept works flawlessly in phantomjs (I am using run-browser https://github.com/ForbesLindesay/run-browser ).
If we get all the nodejs code working well in the browser, it's entirely possible that we could completely deprecate the implementation in lib/js unless someone has a strong reason to maintain that implementation. It's trivial to take the nodejs browser capable implementation and package it up so it produces a standalone file that exposes thrift as a global on the window object if that is something people want.
https://github.com/uber/thrift/commit/9ca44b40504cc8d45d0c1e47709128e474d991df (don't worry I'll break it up into smaller commits before submitting patches))

The tests for this patch are presently writting and reading from the protocol's internal buffer. This is fine, but usually something you can skip if you have end ot end tests working. You may note all of the other tests are end to end (client uses generated IDL code, serializes with proto, sends to transport, transmits to server, and back). This is really a requirement for any protocol, we need to have tests that show it working outright to know it works on initial commit and to avoid regression. After building a test web client and node server to actually use TBinaryProtocol in such a setting I discovered it never writes to the transport (for reference, the write is done in writeMessageEnd() in TJSONProtocol).

I really wanted to get this in prior to jumping onto the several node patches waiting but it is not ready. If you can get the patch to lint clean with JSHint and run the test-async.js test suite clean I'll revisit (see: testws.html for an example).

Best,
Randy

P.S. Andrew de Andrade, I will get the node/compact double patch in next and then start on your list.

Randy Abernethy
added a comment - 02/Feb/15 12:01 Hey Radoslaw Gruchalski ,
The tests for this patch are presently writting and reading from the protocol's internal buffer. This is fine, but usually something you can skip if you have end ot end tests working. You may note all of the other tests are end to end (client uses generated IDL code, serializes with proto, sends to transport, transmits to server, and back). This is really a requirement for any protocol, we need to have tests that show it working outright to know it works on initial commit and to avoid regression. After building a test web client and node server to actually use TBinaryProtocol in such a setting I discovered it never writes to the transport (for reference, the write is done in writeMessageEnd() in TJSONProtocol).
I really wanted to get this in prior to jumping onto the several node patches waiting but it is not ready. If you can get the patch to lint clean with JSHint and run the test-async.js test suite clean I'll revisit (see: testws.html for an example).
Best,
Randy
P.S. Andrew de Andrade , I will get the node/compact double patch in next and then start on your list.

Andrew de Andrade
added a comment - 04/Feb/15 18:35 Radoslaw,
Given your work on TBinaryProtocol here, I figure you may be interested in the work I've done with getting browser support for both XHR and websocket connections in nodejs here:
https://issues.apache.org/jira/browse/THRIFT-2976
I have yet to get the TBinaryProtocol and TCompactProtocol working in the browser, but it's pretty close and I believe the remaining issues are related to how Int64's are handled.

ASF GitHub Bot
added a comment - 24/Aug/16 14:46 Github user ebremer commented on the issue:
https://github.com/apache/thrift/pull/345
Is there any further work being done to support TBinaryProtocol and TCompact in Javascript?

This is the 3rd oldest pull request in the backlog. It would be nice to come to a decision on what to do with this. Does anyone want to pick it up and move forward? Do we want to deprecate lib/js in favor of lib/nodejs and decide not to add this?

James E. King, III
added a comment - 25/Oct/17 13:47 This is the 3rd oldest pull request in the backlog. It would be nice to come to a decision on what to do with this. Does anyone want to pick it up and move forward? Do we want to deprecate lib/js in favor of lib/nodejs and decide not to add this?