Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recent Changes 2019-02-xx #1726

Closed
kazk opened this issue Feb 13, 2019 · 21 comments
Closed

Recent Changes 2019-02-xx #1726

kazk opened this issue Feb 13, 2019 · 21 comments

Comments

@kazk
Copy link
Member

kazk commented Feb 13, 2019

Changes since 2018-12-xx. I haven't been able to work on Codewars recently so it's mostly Code Runner changes.

Feel free to post comments here if you have any questions about the changes.

Code Runner

New Languages ✨

New Language Versions

Other Updates

Codewars

  • Removed many lines!
  • Dependency updates and cleanup
  • Some bug fixes
  • TypeScript 3.3 support
    • Existing kata was updated if compatible
  • JavaScript Node v10.x support
    • See 2018-12-xx for more information and updated wiki page for examples
    • 8.x is still the default for now
    • Planning to run a script to update compatible kata automatically
      • Need to check if tests are written using describe/it properly
      • Found some inconsistent tests (only some assertions are inside it)
  • ReasonML 3.3 support
  • Dart 2.1 support
    • Most of the existing kata was manually updated
    • See Language Dart
  • Java 11 support
    • Existing kata was updated if compatible
  • Groovy 2.5 support
    • Existing kata was updated if compatible
  • Rust 1.33 support
    • Existing kata was updated if compatible
  • Added few wiki pages to improve existing kata:
@kazk kazk pinned this issue Feb 13, 2019
@Voileexperiments
Copy link

Voileexperiments commented Feb 13, 2019

@kazk Since the new Node version adds native BigInt support the existing big integer katas and the likes should have the JS version removed like the Python and Ruby versions. What's the plan for this?

ref: https://v8.dev/blog/bigint

@kazk
Copy link
Member Author

kazk commented Feb 13, 2019

Maybe add something like:

if (!/^v[68]\./.test(process.version)) throw new Error("Only Node 6/8");

to prevent making it available on Node 10+.

Removing a language from a kata is not handled well and I don't think it's worth the risk.

@Chrono79

This comment has been minimized.

@kazk

This comment has been minimized.

@Chrono79

This comment has been minimized.

@FArekkusu
Copy link

Don't know where to report this, so will use this thread.


TypeError: Do not know how to serialize a BigInt
    at JSON.stringify (<anonymous>)

Apparently, BigInt can't be used in the equals-type tests. Small values (like 1n or 100000000n) are fine, but actually big numbers (like 9999999999999999999999999n) lead to the exception.

@Voileexperiments
Copy link

Voileexperiments commented Feb 18, 2019

^ Chai has acknowledged the issue a few months ago but it's on the roadmap to Chai 5, which means it'll probably take a year or two before an official fix will see the light of day.

@kazk There are lots of other issues with Chai:

Array.prototype.foo = 'bar';

describe("Solution", function() {
  it("should test for something", function() {
    Test.assertEquals([2,4,6,8], [2,4,6,9]);
  });
});

In Node 8 it outputs Expected: [2, 4, 6, 9], instead got: [2, 4, 6, 8], but in Node 10 it outputs expected [ 2, 4, 6, 8, foo: 'bar' ] to deeply equal [ 2, 4, 6, 9, foo: 'bar' ]. util.inspect([2, 4, 6, 8]) does not output foo: 'bar' either.

Moreover, util.inspect behaviour can be customized while I don't see how to do this with Chai.

  • Supposedly Chai is calling toJSON, but Date instances are formatting differently than it should be. cw-2.js, Date.prototype.toJSON and util.inspect both formats dates with Date.prototype.toISOString (2019-02-18T02:39:06.346Z) but Chai formats it with Date.prototype.toGMTString (expected Mon, 18 Feb 2019 02:39:06 GMT to equal 3). Milliseconds are not shown so if comparison using deepEqual fails due to millisecond difference, test message will not provide useful information.

  • Chai's inspect library is last updated cough 7 years ago, points references to Node 0.x, and does not use util.inspect in any way, so lots of values makes no sense in the output: e.g WebAssembly and 123n will all become {} as they do not have any enumerable properties. In Node 10 they should be Object [WebAssembly] {} and 123n. It also doesn't support overriding display message with util.inspect.custom.

  • Chai cannot be customized on user's end, so for everything listed above they can't be fixed. Or unless we're doing Test.expect or even printing CW-specific assertion messages manually, but then it defeats the purpose of using a testing framework.

Can we just use the old cw-2.js instead of Chai? There are too many potential pitfalls to consider it viable. cw-2.js is kinda messy but at least it works, and it can be customized. With Chai it's not going to keep up with language changes and it's just dragging everything behind.

@kazk
Copy link
Member Author

kazk commented Feb 18, 2019

Chai is one of the most commonly used assertion libraries and we haven't had any complaints like that from users on Qualified.

That being said, we don't need to use Chai if it's problematic. We can install other assertion library if there's anything better.

  • Chai cannot be customized on user's end, so for everything listed above they can't be fixed. Or unless we're doing Test.expect or even printing CW-specific assertion messages manually, but then it defeats the purpose of using a testing framework.

Can you elaborate on this? I don't understand what you mean by "it defeats the purpose of using a testing framework".

Can we just use the old cw-2.js instead of Chai?

Old cw-2.js cannot be used as is. It needs to be rewritten to throw rather than print the result at least.

@kazk
Copy link
Member Author

kazk commented Feb 18, 2019

For BigInt, it's just not in the current proposal yet so it can't be implemented. Remember that BigInt is still in proposal (even it's in stage 3). See https://github.com/tc39/proposal-bigint

@Voileexperiments
Copy link

Voileexperiments commented Feb 18, 2019

Can you elaborate on this? I don't understand what you mean by "it defeats the purpose of using a testing framework".

What I mean is if the testing framework provides things like assert.deepEqual or assert.equal, I'd expect them to work properly so people will actually use them and not something inappropriate like assert.ok out of "fear of something might break" or something.

That is, yes, assert.ok has its uses, but for it shouldn't be the one that is used by default, and so the use of it should be discouraged; especially since tests that use them need a custom message to make the test meaningful, compared to assert.equal where it's already meaningful with no custom messages. Old katas have Test.expect(actual === expected) all over the place and nobody likes the feedback these kind of tests give to them at all.

Also, util.inspect is the default way of inspecting values in Node (even console.log and the likes use it under the hood), so I'd say chai's behaviour is non-standard here.

@Voileexperiments
Copy link

Voileexperiments commented Feb 18, 2019

Other comments: I'm looking at the current list of installed modules in the Node 10.x environment:

@babel
bignumber.js
chai
cheerio
cw-2
escape-html
esm
espower-loader
express
fast-check
jsdom
lodash
mocha
mongodb
mongoose
pg
power-assert
ramda
redis
sinon
sqlite3
  • Isn't jsdom already deprecated too in favor of cheerio? AFAIK there are only a few katas that need to use them anyway and they're all katas about scraping the CW API, and IIRC jsdom has already been deprecated in these katas
  • I see sinon here. Any plans to reveal its existence soon?

@kazk
Copy link
Member Author

kazk commented Feb 18, 2019

What I mean is if the testing framework provides things like assert.deepEqual or assert.equal, I'd expect them to work properly so people will actually use them

It does work as expected. It's just that output message is different from cw-2 and util.inspect in some edge cases.

Also, util.inspect is the default way of inspecting values in Node (even console.log and the likes use it under the hood), so I'd say chai's behaviour is non-standard here.

util.inspect is not used by console.log. Never mind, I misunderstood this.


Some packages exists for Qualified.

@kazk
Copy link
Member Author

kazk commented Feb 18, 2019

sinon is there since Node 0.10.x days and it's been on the wiki.

@Voileexperiments
Copy link

Voileexperiments commented Feb 18, 2019

One more thing: Chai's inspect behaves differently from before when proxies are involved, e.g:

var o=new Proxy({a:1, b:2, c:3}, {get: (o,v)=>100}), p=new Proxy({a:1, b:2, c:3}, {get: (o,v)=>1});
describe("Solution", function() {
  it("should test for something", function() {
    Test.assertDeepEquals(o, p);
  });
});

Node 8 says Expected: { a: 1, b: 2, c: 3 }, instead got: { a: 1, b: 2, c: 3 }, while Node 10 says expected { a: 100, b: 100, c: 100 } to deeply equal { a: 1, b: 2, c: 3 }. Chai cannot obtain values of the actual object behind the proxy.

Things will only get even worse once ownKeys and the likes are also defined on the proxy handler.

@kazk
Copy link
Member Author

kazk commented Feb 18, 2019

I just tried and got different output.

Node 8:

Expected: { a: 1, b: 2, c: 3 }, instead got: { a: 1, b: 2, c: 3 }

Node 10:

expected { a: 100, b: 100, c: 100 } to deeply equal { a: 1, b: 1, c: 1 }

Test.assertEquals in Node 10 is just assert.deepEqual from Chai.

How is the deep equality of proxied object should be defined? The output of Node 10 looks correct to me because the o.a == 100 and p.a == 1 etc.

@Voileexperiments
Copy link

Voileexperiments commented Feb 18, 2019

The output of Node 10 looks correct to me because the o.a == 100 and p.a == 1 etc.

This is massively complicated once ownKeys and getOwnPropertyDescriptor is overridden, e.g:

var o=new Proxy({a:1, b:2, c:3}, {get: ()=>1, ownKeys: ()=>['a', 'q'], getOwnPropertyDescriptor: ()=>({configurable: true, enumerable: true, value: 'foobar'})}),
    p=new Proxy({a:1, b:2, c:3}, {get: ()=>100, ownKeys: ()=>['a', 'q'], getOwnPropertyDescriptor: ()=>({configurable: true, enumerable: true})});
describe("Solution", function() {
  it("should test for something", function() {
    Test.assertDeepEquals(o, p);
  });
});
  • Node 8: breaks with TypeError: object.value is not a function
  • Node 10: expected { a: 1, q: 1 } to deeply equal { a: 100, q: 100 }
  • util.inspect(o): { a: 'foobar', q: 'foobar' }
  • util.inspect(p): { a: undefined, q: undefined }
  • util.inspect(o, {showProxy: true}): Proxy [ { a: 1, b: 2, c: 3 }, { get: [Function: get], ownKeys: [Function: ownKeys], getOwnPropertyDescriptor: [Function: getOwnPropertyDescriptor] } ]

Trying to enumerate proxies for display purposes is a very bad idea in general because of how easy it can break horribly. Maybe use util.types.isProxy to see if it's a proxy, then use util.inspect(o, {showProxy: true}); for proxies? This seems like the sanest option out of all this madness.

@kazk
Copy link
Member Author

kazk commented Feb 18, 2019

Why should we display like that on test failure for deep equality?

When testing objects with assert.deepEqual(o, p), we shouldn't display if it's a proxy or not because that's not tested. The deep equality doesn't care if actual object is a proxy or not.

If an object needs to be a Proxy that behaves in a certain way, then it should be tested accordingly and not using deepEqual.

@Voileexperiments
Copy link

Voileexperiments commented Feb 18, 2019

Why should we display like that on test failure for deep equality?

Because proxies can reference itself and turn into deep proxies, which makes for a potentially very large, unbounded structure. Proxies are lazy, and so the display should be lazy too, or at least don't go out of bounds easily. util.inspect is designed this way: maxArrayLength: 100 prevents printing the entire array; depth: 2 prevents infinite traversal, getters: false (in Node 11) prevents evaluating getter by default. Circular references is also replaced by [Circular].

With proxies these things can happen:

  • Proxy stores state internally and returns itself every get, so you get a [Circular] (and you'll also mess up with its internal state. It has already happened in a few JS proxy-themed katas)
  • Proxy creates new object per get instead of returning itself, so the serializer ends up traversing the entire tree up to a certain depth

That is, it's not sensible to expand the full content of a proxied object because they can do pretty much anything. Of course, util.inspect straight out getting the target out from the proxy is just as incorrect, so I don't think we should do that either. When people tests proxies they access proxy properties instead of doing a full, dumb Test.assertDeepEquals(proxy, expected), so expanding the proxy would hide the fact that it's actually a proxy, and it makes sense to display proxies as proxy objects instead of its content, which doesn't exist btw.


In addition to this, there are more issues:

  • Chai automatically shortens objects and arrays with a certain display length to a brief display ({ Object (a, b, ...) }, [ Array(8) ]), which is just unhelpful. (Imagine getting a expected { Object (a, b, ...) } to deeply equal { Object (a, b, ...) }; this is about as helpful as Test failed.) The same point above applies: If we have to reinvent our own test messages, we might just as well use assert.ok instead

  • The Chai "Expected | Actual" widget that exists in Node 8 Codewars does not exist anymore in Node 10 Codewars

@Voileexperiments
Copy link

Also,

For BigInt, it's just not in the current proposal yet so it can't be implemented. Remember that BigInt is still in proposal (even it's in stage 3).

BigInt proposal is still in stage 3 because to move a proposal from stage 3 to stage 4 there must be "two implementations by two independent VMs and they must pass the acceptance tests". So I don't see why it can't be implemented; that'd be circular reasoning.

@kazk
Copy link
Member Author

kazk commented Feb 18, 2019

I'll read the previous comment tomorrow, but for BigInt one, I'm talking about its toJSON method only. That's still missing from the proposal as far as I know and I believe they're planning to add it in the future.

@kazk
Copy link
Member Author

kazk commented Feb 18, 2019

We can have assertions and utility package for Codewars that the community can maintain like hspec-codewars. We don't need to worry about backwards compatibility because it won't be made available to previous versions.

If something that handles Proxy better is needed, we can add it there.

  • Chai automatically shortens objects and arrays with a certain display length to a brief display

I wrote how to disable this in the template and also in wiki. Set chai.config.truncateThreshold.

  • The Chai "Expected | Actual" widget that exists in Node 8 Codewars does not exist anymore in Node 10 Codewars

That was part of rendering Chai in cw-2.js. Node 10 Codewars doesn't use cw-2.js. I think we can add a function to enable this when called to the utility package if Chai hasn't changed much.

Can you open a new issue on the runner repo summarizing what the community wants? We can continue there, this is starting to get little off topic.

@kazk kazk unpinned this issue Oct 11, 2020
@kazk kazk closed this as completed May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants