ben-page / node-test

A simple, asynchronous test runner for Node.js.
https://www.npmjs.com/package/node-test
MIT License
4 stars 1 forks source link

Various bugs #5

Open ghost opened 8 years ago

ghost commented 8 years ago

Hey! I saw your latest changes, and this starts to be a awsome runner!! I'm impressed. However I found bugs!

Not a bug, but after looking at AVA and tape source. Why are you passing this from Suite to test, and not keeping this functions within the test module?

Quick question. Why need for a new Runner prototype for the reporter? And how are you going to add in different reporters?

ghost commented 8 years ago

Here is a few more bugs:

Same function is pointing to a non-existing this._err that is never used.

ghost commented 8 years ago

@ben-page I found a few more bugs, and a few other things.

I really enjoy reading your code! Amazing good work! And the result are just stunning!

Keep up the good work!!!!

ghost commented 8 years ago

Due to changes in nodeJS v5.4 you should use 1e9 and normaliize the source array for hrtime. I can't remember exactly where I was reading that, but see NodejS documentation: https://nodejs.org/api/process.html#process_process_hrtime_time

They are also using 1e9

ben-page commented 8 years ago

use of instanceOf causes a DEOPT in Chrome and should be avoided.

Will you provide a source for this? I can't find anyone recommending that instanceOf be avoided.

ghost commented 8 years ago

I spoke with a colleague of mine and this DEOPT was caused by hidden classes etc with V8 that Chrome uses. He should search for links regarding this