search

metarhia / metatests

Extremely simple to use test framework and runner for Metarhia technology stack 🧪
https://metarhia.com
Other
18 stars 4 forks source link

Use util.inspect instead of JSON.stringify #64

Closed lundibundi closed 6 years ago

lundibundi commented 6 years ago

This will fix https://github.com/metarhia/metatests/issues/63. Also in the current 'declarative' checker, you can do const f = x => x and [Number.NaN, null] it will not fail. This PR fixes that. Though I wanted to add a negative declarative test case but couldn't finds a way to do it, is it possible?

I ran common tests and found a bug. Also tried globalstorage tests but it just hang indefinitely on mongo provider tests, do I need anything special (besides mongodb) to run them?

lundibundi commented 6 years ago

@tshemsedinov @belochub ping.

lundibundi commented 6 years ago

Also, @tshemsedinov what do you think of replacing our own compare/strictEqual with Node core util.isDeepStrictEqual(val1, val2) as it seems to support all of the checks we support and also takes care of circular references, proper buffer comparison and more (https://github.com/nodejs/node/blob/master/lib/internal/util/comparisons.js)? Good idea but we have to support running tests in browser therefore not really applicable (unless we directly copy code from node.js core util.js)

lundibundi commented 6 years ago

Blocking this before https://github.com/metarhia/metatests/pull/66. It's much easier to refactor this one than dealing with conflicts in #66.

lundibundi commented 6 years ago

Ping @belochub @tshemsedinov this is now ready.

nechaido commented 6 years ago

@lundibundi we will need to run metatests in browser. We should maybe write our own function instead of using Node.js's util.inspect and polyfill for it. WDYT?

lundibundi commented 6 years ago

Well, it is reasonable, though it's quite hard to implement such a function so that it will properly be usable for comparison and display of the results. I'd say to add guards for browser (use JSON.stringify there) for now and in a follow-up PR actually implement it and move away from using util.inspect. wdyt?

nechaido commented 6 years ago

@lundibundi ok.

belochub commented 6 years ago

@lundibundi what's the point of this PR then, as it just replaces one wrong way of checking for equality with another? I think we should start with implementing a new comparison function.

lundibundi commented 6 years ago

Well, this way of checking the equality at least works properly and properly shows the difference (even though only in node). And for the browser we will implement a new one. Rightn now we want to release a next version and I think it'd be better if it at least worked correctly in node.

belochub commented 6 years ago

@lundibundi According to Node.js documentation:

The util.inspect() method returns a string representation of object that is intended for debugging. The output of util.inspect may change at any time and should not be depended upon programmatically.

I think we shouldn't use this method for value comparison; it is not an intended or appropriate use of this method.

We've already released a new version today, we can implement a better way of comparison instead of this one and publish it in the next version, I don't think there's a need to add some kludges to speed up the process of releasing the new version.

lundibundi commented 6 years ago

Oh well, that's unfortunate, I thought I could make it in time. Ok then.