chakrashim, test: Chakrashim fixes

Below things were fixed in chakrashim * There was unnecessary assert added in previous commit for `PropertyDescriptor`. Removed it. * We wanted to cache `Object.getOwnPropertyDescriptor` but while initializing the shim we did by extracting `getOwnPropertyDescriptor` property from `Object.prototype` which was `undefined`. This was not broken so far because it was never used. Node started using this feature and hence realized it. Fixed it by calling `JsGetOwnPropertyDescriptor` instead. This has performance hit slightly because now property names has to be converted to property id before calling Jsrt api. Earlier this was taken care by runtime. Below things were fixed for unit test * Error message difference * There was a test case marked as known issue but that doesn't fail for chakracore. Skipping its execution so it doesn't get tagged as FAILED.
nodejs · Feb 26, 2017 · ed2964b · ed2964b
1 parent 233a053
commit ed2964b
Show file tree

Hide file tree

Showing 14 changed files with 73 additions and 40 deletions.
diff --git a/deps/chakrashim/src/jsrtcontextcachedobj.inc b/deps/chakrashim/src/jsrtcontextcachedobj.inc
@@ -47,7 +47,6 @@ DEFTYPE(Float64Array)
 
 
 // These prototype functions will be cached/shimmed
-DEFMETHOD(Object,         getOwnPropertyDescriptor)
 DEFMETHOD(Object,         hasOwnProperty)
 DEFMETHOD(Object,         toString)
 DEFMETHOD(String,         concat)

diff --git a/deps/chakrashim/src/jsrtcontextshim.cc b/deps/chakrashim/src/jsrtcontextshim.cc
@@ -549,9 +549,12 @@ DECLARE_GETOBJECT(DateConstructor,
                   globalConstructor[GlobalType::Date])
 DECLARE_GETOBJECT(ProxyConstructor,
                   globalConstructor[GlobalType::Proxy])
-DECLARE_GETOBJECT(GetOwnPropertyDescriptorFunction,
+DECLARE_GETOBJECT(HasOwnPropertyFunction,
                   globalPrototypeFunction[GlobalPrototypeFunction
-                      ::Object_getOwnPropertyDescriptor])
+                    ::Object_hasOwnProperty])
+DECLARE_GETOBJECT(ToStringFunction,
+                  globalPrototypeFunction[GlobalPrototypeFunction
+                    ::Object_toString])
 DECLARE_GETOBJECT(StringConcatFunction,
                   globalPrototypeFunction[GlobalPrototypeFunction
                     ::String_concat])

diff --git a/deps/chakrashim/src/jsrtcontextshim.h b/deps/chakrashim/src/jsrtcontextshim.h
@@ -76,7 +76,9 @@ class ContextShim {
   JsValueRef GetRegExpConstructor();
   JsValueRef GetProxyConstructor();
   JsValueRef GetGlobalType(GlobalType index);
-  JsValueRef GetGetOwnPropertyDescriptorFunction();
+
+  JsValueRef GetHasOwnPropertyFunction();
+  JsValueRef GetToStringFunction();
   JsValueRef GetStringConcatFunction();
   JsValueRef GetArrayPushFunction();
   JsValueRef GetGlobalPrototypeFunction(GlobalPrototypeFunction index);

diff --git a/deps/chakrashim/src/jsrtutils.cc b/deps/chakrashim/src/jsrtutils.cc
@@ -273,8 +273,7 @@ JsErrorCode HasOwnProperty(JsValueRef object,
                            JsValueRef prop,
                            JsValueRef *result) {
   JsValueRef hasOwnPropertyFunction =
-    ContextShim::GetCurrent()->GetGlobalPrototypeFunction(
-      ContextShim::GlobalPrototypeFunction::Object_hasOwnProperty);
+    ContextShim::GetCurrent()->GetHasOwnPropertyFunction();
 
   JsValueRef args[] = { object, prop };
   return JsCallFunction(hasOwnPropertyFunction, args, _countof(args), result);
@@ -283,9 +282,16 @@ JsErrorCode HasOwnProperty(JsValueRef object,
 JsErrorCode GetOwnPropertyDescriptor(JsValueRef ref,
                                      JsValueRef prop,
                                      JsValueRef* result) {
-  return CallFunction(
-    ContextShim::GetCurrent()->GetGetOwnPropertyDescriptorFunction(),
-    ref, prop, result);
+
+  JsPropertyIdRef idRef;
+  JsErrorCode error;
+
+  error = GetPropertyIdFromName(prop, &idRef);
+  if (error != JsNoError) {
+    return error;
+  }
+
+  return JsGetOwnPropertyDescriptor(ref, idRef, result);
 }
 
 JsErrorCode IsZero(JsValueRef value,

diff --git a/deps/chakrashim/src/v8object.cc b/deps/chakrashim/src/v8object.cc
@@ -510,8 +510,7 @@ bool Object::SetPrototype(Handle<Value> prototype) {
 
 MaybeLocal<String> Object::ObjectProtoToString(Local<Context> context) {
   ContextShim* contextShim = ContextShim::GetCurrent();
-  JsValueRef toString = contextShim->GetGlobalPrototypeFunction(
-    ContextShim::GlobalPrototypeFunction::Object_toString);
+  JsValueRef toString = contextShim->GetToStringFunction();
 
   JsValueRef result;
   JsValueRef args[] = { this };

diff --git a/deps/chakrashim/src/v8propertydescriptor.cc b/deps/chakrashim/src/v8propertydescriptor.cc
@@ -106,12 +106,10 @@ Local<Value> PropertyDescriptor::value() const {
 }
 
 Local<Value> PropertyDescriptor::get() const {
-  CHAKRA_ASSERT(private_->has_get());
   return Local<Value>::New(private_->get);
 }
 
 Local<Value> PropertyDescriptor::set() const {
-  CHAKRA_ASSERT(private_->has_set());
   return Local<Value>::New(private_->set);
 }
 

diff --git a/test/known_issues/known_issues.status b/test/known_issues/known_issues.status
@@ -21,4 +21,5 @@ test-stdout-buffer-flush-on-exit: SKIP
 
 [$jsEngine==chakracore]
 test-vm-function-redefinition : SKIP
-test-vm-inherited_properties : SKIP
+test-vm-inherited_properties : SKIP
+test-vm-data-property-writable : SKIP
diff --git a/test/message/error_exit.chakracore.out b/test/message/error_exit.chakracore.out
@@ -1,5 +1,5 @@
 Exiting with code=1
-AssertionError: 1 == 2
+AssertionError: 1 === 2
    at Anonymous function (*test*message*error_exit.js:*:*)
    at Module.prototype._compile (module.js:*:*)
    at Module._extensions[.js] (module.js:*:*)

diff --git a/test/parallel/test-http-outgoing-proto.js b/test/parallel/test-http-outgoing-proto.js
@@ -1,5 +1,5 @@
 'use strict';
-require('../common');
+const common = require('../common');
 const assert = require('assert');
 
 const http = require('http');
@@ -62,7 +62,10 @@ assert.throws(() => {
 assert.throws(() => {
   const outgoingMessage = new OutgoingMessage();
   outgoingMessage.addTrailers();
-}, /^TypeError: Cannot convert undefined or null to object$/);
+}, common.engineSpecificMessage({
+        v8: /^TypeError: Cannot convert undefined or null to object$/,
+        chakracore: /^TypeError: Object expected/
+}));
 
 assert.throws(() => {
   const outgoingMessage = new OutgoingMessage();

diff --git a/test/parallel/test-internal-util-decorate-error-stack.js b/test/parallel/test-internal-util-decorate-error-stack.js
@@ -24,7 +24,10 @@ assert.strictEqual(obj.stack, undefined);
 
 // Verify that the stack is decorated when possible
 function checkStack(stack) {
-  const matches = stack.match(/var foo bar;/g);
+  const matches = stack.match(common.engineSpecificMessage({
+    v8: /var foo bar;/g,
+    chakracore: /Expected ';'/g  // chakra does not show source
+  }));
   assert.strictEqual(Array.isArray(matches), true);
   assert.strictEqual(matches.length, 1);
 }

diff --git a/test/parallel/test-url-format-whatwg.js b/test/parallel/test-url-format-whatwg.js
@@ -1,6 +1,6 @@
 'use strict';
 
-require('../common');
+const common = require('../common');
 const assert = require('assert');
 const url = require('url');
 const URL = url.URL;
@@ -76,20 +76,24 @@ assert.strictEqual(
   'http://xn--lck1c3crb1723bpq4a.com/a?a=b#c'
 );
 
-assert.strictEqual(
+// Unicode conversion in node code is dependent on
+// v8's i18n support which is disabled for chakracore
+if (!common.isChakraEngine) {
+  assert.strictEqual(
   url.format(myURL, {unicode: true}),
   'http://理容ナカムラ.com/a?a=b#c'
-);
-
-assert.strictEqual(
-  url.format(myURL, {unicode: 1}),
-  'http://理容ナカムラ.com/a?a=b#c'
-);
-
-assert.strictEqual(
-  url.format(myURL, {unicode: {}}),
-  'http://理容ナカムラ.com/a?a=b#c'
-);
+  );
+
+  assert.strictEqual(
+    url.format(myURL, {unicode: 1}),
+    'http://理容ナカムラ.com/a?a=b#c'
+  );
+
+  assert.strictEqual(
+    url.format(myURL, {unicode: {}}),
+    'http://理容ナカムラ.com/a?a=b#c'
+  );
+}
 
 assert.strictEqual(
   url.format(myURL, {unicode: false}),

diff --git a/test/parallel/test-whatwg-url-properties.js b/test/parallel/test-whatwg-url-properties.js
@@ -1,7 +1,7 @@
 // Flags: --expose-internals
 'use strict';
 
-require('../common');
+const common = require('../common');
 const URL = require('url').URL;
 const assert = require('assert');
 const urlToOptions = require('internal/url').urlToOptions;
@@ -44,8 +44,14 @@ assert.strictEqual(url.searchParams, oldParams);  // [SameObject]
 // non-writable property should throw.
 // Note: this error message is subject to change in V8 updates
 assert.throws(() => url.origin = 'http://foo.bar.com:22',
-              new RegExp('TypeError: Cannot set property origin of' +
-                         ' \\[object URL\\] which has only a getter'));
+              new RegExp(
+                common.engineSpecificMessage({
+                  v8: 'TypeError: Cannot set property origin of' +
+                         ' \\[object URL\\] which has only a getter',
+                  chakracore: 'TypeError: Assignment to read-only' +
+                          ' properties is not allowed in strict mode'
+                })
+              ));
 assert.strictEqual(url.origin, 'http://foo.bar.com:21');
 assert.strictEqual(url.toString(),
                    'http://user:[email protected]:21/aaa/zzz?l=25#test');
@@ -120,8 +126,14 @@ assert.strictEqual(url.hash, '#abcd');
 // non-writable property should throw.
 // Note: this error message is subject to change in V8 updates
 assert.throws(() => url.searchParams = '?k=88',
-              new RegExp('TypeError: Cannot set property searchParams of' +
-                         ' \\[object URL\\] which has only a getter'));
+              new RegExp(
+                common.engineSpecificMessage({
+                  v8: 'TypeError: Cannot set property searchParams of' +
+                         ' \\[object URL\\] which has only a getter',
+                  chakracore: 'TypeError: Assignment to read-only properties' +
+                         ' is not allowed in strict mode'
+                })
+              ));
 assert.strictEqual(url.searchParams, oldParams);
 assert.strictEqual(url.toString(),
                    'https://user2:[email protected]:23/aaa/bbb?k=99#abcd');

diff --git a/test/sequential/test-fs-readfile-tostring-fail.js b/test/sequential/test-fs-readfile-tostring-fail.js
@@ -31,6 +31,11 @@ stream.end();
 stream.on('finish', common.mustCall(function() {
   // make sure that the toString does not throw an error
   fs.readFile(file, 'utf8', common.mustCall(function(err, buf) {
+    // Skip 'toString()' check for chakra engine because it verifies limit of v8
+    // specific kStringMaxLength variable.
+    if (common.isChakraEngine) {
+      return;
+    }
     assert.ok(err instanceof Error);
     assert.strictEqual('"toString()" failed', err.message);
     assert.strictEqual(buf, undefined);

diff --git a/test/sequential/test-module-loading.js b/test/sequential/test-module-loading.js
@@ -73,13 +73,11 @@ assert.throws(
   function() {
     require('../fixtures/packages/invalid');
   },
-  new RegExp(
-    '^SyntaxError: Error parsing \\S+: ' +
+  new RegExp('^SyntaxError: Error parsing \\S+: ') +
     common.engineSpecificMessage({
       v8: 'Unexpected token , in JSON at position 1$',
-      chakraCore: 'JSON.parse Error: Invalid character at position:2$'
+      chakracore: 'JSON.parse Error: Invalid character at position:2$'
     })
-  )
 );
 
 assert.strictEqual(require('../fixtures/packages/index').ok, 'ok',