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

JS Interpreter: Fix incorrect sort() behavior #19012

Merged
merged 4 commits into from Nov 9, 2017
Merged

Conversation

@islemaster
Copy link
Member

islemaster commented Nov 9, 2017

A Zendesk ticket today revealed that Array.sort() in App Lab is broken in some cases:

// Array.sort doesn't work properly
console.log([5, 4, 5, 1].sort());
// Expected: [1,4,5,5]
// Actual  : [4,1,5,5]

I've updated our interpreter to a new version that includes a fix for the sort behavior. I've also added a regression test for the broken case, and along the way, I added a new test utility that (in my opinion) makes certain App Lab tests much nicer to read and write.

import tickWrapper from './tickWrapper';
import {TestResults} from '@cdo/apps/constants';

export function testApplabConsoleOutput({testName, source, expect, ticks=2}) {

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Nov 9, 2017

Contributor

worth adding param tags for these?

This comment has been minimized.

Copy link
@islemaster

islemaster Nov 9, 2017

Author Member

Yeah, probably. I figured it might be nice to build up a collection of level test generators, for common cases like this. Documenting the first one well is probably an important step. 😆

This comment has been minimized.

Copy link
@davidsbailey

davidsbailey Nov 9, 2017

Contributor

nice abstraction!

Copy link
Contributor

davidsbailey left a comment

LGTM

import tickWrapper from './tickWrapper';
import {TestResults} from '@cdo/apps/constants';

export function testApplabConsoleOutput({testName, source, expect, ticks=2}) {

This comment has been minimized.

Copy link
@davidsbailey

davidsbailey Nov 9, 2017

Contributor

nice abstraction!

@islemaster islemaster merged commit d2affb8 into staging Nov 9, 2017
1 check passed
1 check passed
DTS The staging branch is open.
@islemaster islemaster deleted the interpreter-wut branch Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.