nodejs / node-chakracore

Node.js on ChakraCore :sparkles::turtle::rocket::sparkles:
Other
1.92k stars 340 forks source link

test: enable bigint tests on v8 #593

Closed kfarnung closed 6 years ago

kfarnung commented 6 years ago
Checklist
kfarnung commented 6 years ago

CI: https://ci.nodejs.org/job/chakracore-test-pull-request/315/

IrinaYatsenko commented 6 years ago

Isn't it useful to run the tests for non-bigInt cases? I did it for the pump in d4d1ef76f7. I'm fine with eval as well, but it likely will be a conflict.

kfarnung commented 6 years ago

I'm assuming these cases are covered elsewhere, these are specifically about testing bigint. This is based on the current master, so there should be no conflict.

kfarnung commented 6 years ago

CI rerun: https://ci.nodejs.org/job/chakracore-test-pull-request/316/

IrinaYatsenko commented 6 years ago

If I understand it correctly, the tests validate equivalence of various types when used as keys/values (e.g. null and undefined). BigInts were added as yet another equivalence test.

kfarnung commented 6 years ago

Correct, but if we don't support bigints then the test isn't necessary for node-chakracore anyway.

IrinaYatsenko commented 6 years ago

Aren't you disabling the tests for undefined, null and falsies (as well as bigInt)? I don't like it that all of them are in the same bucket, should probably be separate (that, or I don't understand the tests).

kfarnung commented 6 years ago

Oh, I see now, it looks like they conflated these tests a bit. I think I'd rather just disable them rather than trying to duplicate their behavior without BigInt. That will keep merge conflicts to a minimum in the future.

IrinaYatsenko commented 6 years ago

OK. I'm fine with that.

joaocgreis commented 6 years ago

Rebuild of the Windows job: https://ci.nodejs.org/job/chakracore-test-windows/492/ (failed in the test-PR above for unrelated reasons).