tc39 / test262

Official ECMAScript Conformance Test Suite
Other
2.37k stars 461 forks source link

Invalid assertion in `test/built-ins/RegExp/S15.10.2.10_A5.1_T1.js` #858

Closed demurgos closed 7 years ago

demurgos commented 7 years ago

Hi, I think that the following line in test/built-ins/RegExp/S15.10.2.10_A5.1_T1.js do not use the right signature for assert.notSameValue:

assert.notSameValue(null, "No match for character: " + non_ident[k]);

The signature is notSameValue(actual, expected, message) but here it is called as notSameValue(expected, message).

The correct form should be:

assert.notSameValue(arr, null, "No match for character: " + non_ident[k]);
littledan commented 7 years ago

Great catch! It's easy to miss bugs like these because this sort of condition will always be true. I uploaded a new patch for a fix.

I really appreciate your help here. Filing bugs on its own is always welcome, but also, if you want to write your own patches for test262, see the readme for how to get started. PRs welcome (just as long as the CLA is signed)!

demurgos commented 7 years ago

Ok, thank you for patching it.

For the moment I am just looking around and checking the RegExp parts. I think that I found some patterns that should lead to a syntax error according to the spec but are accepted by some browsers (/]/, a(?!b){2}, etc.) so I am just checking what are the tests you currently have (maybe I am wrong). Once I am more familiar with this part of the spec, I think that I'll submit some additional tests.

By the way, I see that you currently number the tests according to the revision 5.1 of ECMA-262 (at least for the RegExp). What is the current opinion about using the version 7.0 since it is the last "stable" revision of the spec ?

littledan commented 7 years ago

test262 targets the current draft spec plus Stage 3 and 4 proposals. It does not maintain tests against historical ECMAScript versions.

For those syntax errors, are you familiar with Annex B 1.4? I believe it makes both of those examples legal. There were some additions to it in ES2016.

demurgos commented 7 years ago

Thanks!

I effectively was missing the part about the browser specificities.

littledan commented 7 years ago

Yeah, we have discussed including tests for the non-Annex B environment, where this syntax does not exist, but we don't currently have tests like that in test262, and there isn't a native JS implementation with this behavior restriction either

demurgos commented 7 years ago

Fixed in #859