codewars / runner

Issue tracker for Code Runner
34 stars 8 forks source link

Add CoffeeScript 2.7.0 #277

Open KayleighWasTaken opened 10 months ago

KayleighWasTaken commented 10 months ago

The main rationale behind requesting this update is due to the extremely outdated (and slow) Node version (v 8.7.0) that the current CS 1.10.0 runner is using. This will also make transitioning away from the Codewars test framework to the chai framework used by the JS/TS runners easier, as chai assertions (and assertions in general) behave very poorly on Node v8, pictured below.

An assertion being run on the Node v8 JS/CS runner: image

The same assertion being run on the Node v10 JS runner: image

This is an issue as for Kata with large expected outputs, for example The Observed Pin, where the output buffer can become entirely filled without having chai truncate the test output and giving the solver unhelpful error messages such as expected [ Array(1440) ] to deeply equal [ Array(1440) ]. The performance of certain assertions also seems to be severely degraded on the current runner, as a single deep equality check between two ~30000 element arrays takes over 12000ms on its own.

KayleighWasTaken commented 10 months ago

If updating to CS 2+ is currently out of scope, then simply switching over to the current latest JS Node runner (v18.x) will give significantly improved assertion behaviour and performance.

KayleighWasTaken commented 10 months ago

If updating to CS 2+ is currently out of scope, then simply switching over to the current latest JS Node runner (v18.x) will give significantly improved assertion behaviour and performance.

The actual performance improvement of the v18.x runner over the v8.7.0 runner is more than an order of magnitude when working with specific assertions. Using the compiled JS output of this translation in a kumite (with random test specifications changed to match other languages rather than the limited ones implemented for the Node v8 runner) gives a speedup from timeout to ~200ms runtime.

To verify my findings simply fork the kumite and run the tests with different Node runners selected.

kazk commented 10 months ago

This will also make transitioning away from the Codewars test framework to the chai framework

I know this is confusing, but Chai is an assertion package and not a test framework. Mocha is the test framework used in Node 10+ and TypeScript. It provides describe/it, and runs the tests. Chai is the most commonly used assertion package and also used by our package that provides some compatibility (Test.*) for Node 10+.

I'm sure the newer Node version is faster, but you can't really compare the performance of tests between Node v8 and v18 on Codewars because Node v8 is not using Mocha+Chai.

The performance of certain assertions also seems to be severely degraded on the current runner, as a single deep equality check between two ~30000 element arrays takes over 12000ms on its own.

Are you comparing the performance of Test.assertDeepEquals? I'd guess it's most likely because the custom test framework used in Node v8 and CoffeeScript uses lodash's isEqual under the hood instead of Chai. The Test.assertDeepEquals in Node 10+ is assert.deepEqual from Chai.

as chai assertions (and assertions in general) behave very poorly on Node v8, pictured below.

As I wrote in #233, the difference for the test output is the test framework. In Kumite, you can try Mocha with Node v8 by changing the test framework on the bottom right.


If we decide to add CoffeeScript 2, we'll use Mocha and the latest LTS Node.

KayleighWasTaken commented 10 months ago

Are you comparing the performance of Test.assertDeepEquals? I'd guess it's most likely because the custom test framework used in Node v8 and CoffeeScript uses lodash's isEqual under the hood instead of Chai. The Test.assertDeepEquals in Node 10+ is assert.deepEqual from Chai.

I am comparing chai.assert.deepEqual in both versions, chai is an available package for the CS 1.10.0 runner. The performance of assert.deepEqual in v8/v10 vs v18 (all on mocha) also about an order of magnitude slower.

kazk commented 10 months ago

Cool. CoffeeScript has Chai v3.5.0. Node v12/v14/v18 has Chai v4.3.x.

kazk commented 10 months ago

deep equality has been rewritten from the ground up to support ES6 types like Map and Set, and better support existing types. It is now also much, much faster than before and allows us to bring some great improvements in upcoming releases. https://github.com/chaijs/chai/releases/tag/4.0.0

KayleighWasTaken commented 10 months ago

@kazk @ejini6969 was wondering when any potential updates to CS will commence and if there is anything from our side that can help (PRs or similar)? He says he's planning on updating all current CS kata to use Chai assertions this weekend as well, and is wondering if that is needed or it can be automated from your end?

kazk commented 10 months ago

To maximize future compatibility with Mocha, make sure to have the valid test structure and use nullary functions. See the list at the top of List of JavaScript Kata to Update for more details. We can provide assertions from Test.* that uses Chai with our package like JavaScript, and we can easily remove the Test. prefix from Test.describe/Test.it, but it's difficult to automatically fix the lack of test structure.

Also, make sure the tests are not using Node's assert that's imported implicitly. See how the current test file is created (https://github.com/codewars/runner/issues/233#issuecomment-1419972310).

ejini6969 commented 10 months ago

Was aware of those remarks! Currently using 1 and 2 as reference for update. Both have different test structures but compatible with the requirements.