Jens Geyer created THRIFT-6322:
----------------------------------
Summary: lib/js: the browser tests call struct read and write as
plain properties
Key: THRIFT-6322
URL: https://issues.apache.org/jira/browse/THRIFT-6322
Project: Thrift
Issue Type: Bug
Components: JavaScript - Library
Reporter: Jens Geyer
{{lib/js/test/deep-constructor.test.js}} fails four of its assertions:
{code}
>> Serialize/deserialize simple struct should return equal object
>> Message: Died on test #1 ... data.write is not a function
>> TypeError: data.write is not a function
>> at serialize (deep-constructor.test.js:24)
5 tests completed with 4 failed, 0 skipped, and 0 todo.
72 assertions, passed: 68, failed: 4
{code}
The JavaScript generator keys the struct methods by symbol:
{code}
Simple.prototype[Symbol.for("read")] = function(input) { ... };
Simple.prototype[Symbol.for("write")] = function(output) { ... };
{code}
and generated code calls them the same way, for example in
{{gen-js/ThriftTest_types.js}}:
{code}
this.struct_thing[Symbol.for("read")](input);
this.struct_thing[Symbol.for("write")](output);
{code}
The tests still use plain property access. Every site, from a sweep of the
browser test
code for both spellings:
|| File || Line || Call ||
| {{lib/js/test/deep-constructor.test.js}} | 24 | {{data.write(protocol)}} |
| {{lib/js/test/deep-constructor.test.js}} | 35 | {{data.read(protocol)}} |
| {{lib/js/test/test.js}} | 244 | {{xtruct3.read(input)}} |
| {{lib/js/test/phantom-client.js}} | 268 | {{xtruct3.read(input)}} |
Nothing else in the library or in the generated code does: those all go through
{{Symbol.for(...)}}.
h2. Why nobody noticed
The test has not run. {{lib/js/Makefile.am}} guards {{check-local}} with {{if
HAVE_NPM}}, and
{{configure.ac}} looks for npm only inside the {{--with-nodejs}} and
{{--with-nodets}} blocks.
Any build that disables the nodejs binding leaves {{NPM}} empty, so {{make -C
lib/js check}}
reports "Nothing to be done" and exits 0. THRIFT-6319 corrects that, which is
what brought this
failure to light.
h2. Verified
Reproduced on master {{82bab6306}}, and with the generator changes of
{{THRIFT-6317}} and
{{THRIFT-6318}} reverted -- the failure is identical either way, so it does not
come from those.
Getting this far also needs the jshint language levels corrected, otherwise
{{jshint:test}}
aborts the run before the tests.
Switching the two sites in {{deep-constructor.test.js}} to {{Symbol.for(...)}}
turns that page
green -- 5 tests, 75 assertions, 0 failed -- and the run then advances to
{{test-nojq.html}},
which fails on the {{test.js}} site with the same message. So the list above is
the whole fix.
Drafted with AI assistance (Claude Opus 5).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
